Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add comparison processes #50

Merged
merged 51 commits into from
May 19, 2023

Conversation

ValentinaHutter
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #50 (193bf1c) into main (64b00b9) will increase coverage by 1.40%.
The diff coverage is 94.66%.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   75.05%   76.46%   +1.40%     
==========================================
  Files          23       24       +1     
  Lines         886      956      +70     
==========================================
+ Hits          665      731      +66     
- Misses        221      225       +4     
Impacted Files Coverage Δ
...eo_processes_dask/process_implementations/utils.py 88.88% <88.88%> (ø)
...ocesses_dask/process_implementations/comparison.py 94.82% <94.82%> (ø)
...processes_dask/process_implementations/__init__.py 75.00% <100.00%> (+1.66%) ⬆️
...cesses_dask/process_implementations/cubes/utils.py 86.66% <100.00%> (+11.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ValentinaHutter ValentinaHutter changed the title Update pre-commit hook Comparison processes Jan 30, 2023
Copy link
Collaborator

@SerRichard SerRichard left a comment

Choose a reason for hiding this comment

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

lgtm, only comment would be to try and exhaust all conditional cases. Not so familiar with the processes at this point, but the test set up looks good!

@LukeWeidenwalker LukeWeidenwalker changed the title Comparison processes feat: add comparison processes Feb 24, 2023
@clausmichele
Copy link
Member

What's the status of this PR? Can I help to proceed with this?

@LukeWeidenwalker
Copy link
Contributor

Let's look into this after the SRR!

or isinstance(y, bool)
and not isinstance(x, bool)
):
return False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#50 (comment)

I now added this check to make sure that the example of eq(x = 0, y = false) => false works, but there is one problem left:
I do this check for inputs that only consist of one value, e.g. int, float, bool. As soon as I want to do this for an array and check if it is a bool array, it might not work, because numpy easily converts a bool array into a float array. In that case, False would be set to 0 and the example would not work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@LukeWeidenwalker I tried implementing the eq(x = 0, y = false) => false example with an explicit check for the bool type if the input value only consists of one number - as soon as there are numpy or dask arrays, I would not check it explicitely, is this fine for you? or should I add a check for the numpy array dtype?

@ValentinaHutter
Copy link
Collaborator Author

The current version shows one problem that now occured, as I tested merge_cubes with the overlap_resolver eq: The result of the merge_cubes process is of type xr.DataArray and loads the data into memory. I am not sure, why this happens within merge_cubes, as the eq process works as expected with apply. Need to investigate further.

@LukeWeidenwalker
Copy link
Contributor

Btw, I just realised, comparison of temporal stuff is deprecated with 2.0: Open-EO/PSC#21, so we don't need to bother implementing this! :)

@ValentinaHutter
Copy link
Collaborator Author

Btw, I just realised, comparison of temporal stuff is deprecated with 2.0: Open-EO/PSC#21, so we don't need to bother implementing this! :)

Ah, thanks for pointing this out, then I will quickly update the check for the datetime again!

@clausmichele
Copy link
Member

Hi @ValentinaHutter and @LukeWeidenwalker, today it would be the last day to merge and make available the comparison processes and let me prepare the notebook for Monday

@LukeWeidenwalker
Copy link
Contributor

LukeWeidenwalker commented May 19, 2023

Hi @ValentinaHutter and @LukeWeidenwalker, today it would be the last day to merge and make available the comparison processes and let me prepare the notebook for Monday

Yesterday was a public holiday in Austria, so I just saw this! @ValentinaHutter made more changes to this on Wednesday, which I'll review and iterate on again today. However, I don't think you really need us to merge&release these changes to use them. If you're happy with the current level of implementation, couldn't you just grab the code and stick it into your process registry manually?

@clausmichele
Copy link
Member

Unfortunately it's not a viable option, I would have to ask 20+ users to install a non released version from source and I wouldn't have the time for testing it now.

Copy link
Contributor

@LukeWeidenwalker LukeWeidenwalker left a comment

Choose a reason for hiding this comment

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

@ValentinaHutter I've made a few more changes, and found another bug in merge_cubes! Just FYI, not_null wasn't dask-friendly yet, so now I was able to replace da.where with np.where again.

Think this looks good now, thanks for adding all the tests, these were really useful to have in place already! :)

@clausmichele: I'll merge this now and cut a release for you to use in your demo!

@LukeWeidenwalker LukeWeidenwalker merged commit 4771048 into main May 19, 2023
@LukeWeidenwalker LukeWeidenwalker deleted the EODCAPI-184-complete-comparison-processes branch May 19, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants