Skip to content
This repository has been archived by the owner on Dec 11, 2022. It is now read-only.

Various quality of life improvements #50

Merged
merged 19 commits into from
Feb 10, 2021
Merged

Various quality of life improvements #50

merged 19 commits into from
Feb 10, 2021

Conversation

tpoisot
Copy link
Member

@tpoisot tpoisot commented Feb 5, 2021

What the pull request does

Type of change

Please indicate the relevant option(s)

  • ❇️ New feature (non-breaking change which adds functionality)
  • 📖 This change requires a documentation update

Checklist

  • The changes are documented
    • The docstrings of the different functions describe the arguments, purpose, and behavior
    • There are examples in the documentation website
  • The changes are tested
  • The changes do not modify the behavior of the previously existing functions
  • The Project.toml field version has been updated

@codecov-io
Copy link

codecov-io commented Feb 5, 2021

Codecov Report

Merging #50 (f899850) into master (1a41008) will increase coverage by 1.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   85.75%   87.05%   +1.29%     
==========================================
  Files          17       17              
  Lines         344      363      +19     
==========================================
+ Hits          295      316      +21     
+ Misses         49       47       -2     
Flag Coverage Δ
unittests 87.05% <100.00%> (+1.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/datasets/geotiff.jl 100.00% <100.00%> (ø)
src/lib/overloads.jl 94.44% <100.00%> (+4.30%) ⬆️
src/recipes/recipes.jl 88.23% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a41008...8b3df35. Read the comment docs.

@tpoisot tpoisot changed the title Add a collect method Various quality of life improvements Feb 5, 2021
@tpoisot tpoisot marked this pull request as ready for review February 9, 2021 01:21
Copy link
Member

@gabrieldansereau gabrieldansereau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the features! I've played a bit with them on some of my codes and everything seems fine.

Very minor gripe about the isnothing overload however, which I find confusing. I would expect that behaviour with a broadcasted call like isnothing.(layer), not isnothing(layer), since it's operating on the grid elements. Right now it doesn't mimic the same calls made on an Array, as we've done elsewhere. Since it's a bit counterintuitive, I'd just use the one-line call on the grid directly.

Also, you could want to check if a layer is an element of type nothing when calling isnothing(layer), as you can with any other element. But I admit that's not very likely.

So I'm not really opposed to it, I just find it a bit inconsistent and unnecessary.

@tpoisot
Copy link
Member Author

tpoisot commented Feb 9, 2021

@gabrieldansereau you might be right - I think we can remove it and use an iterator as suggested by @mkborregaard in #51, to do something with findall instead. I should be able to remove it without too much trouble.

@tpoisot tpoisot merged commit ed1eef5 into master Feb 10, 2021
@gabrieldansereau gabrieldansereau deleted the tpoisot/issue48 branch February 17, 2021 20:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add hcat and vcat Add a collect or values method Add a new method to similar
3 participants