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

Refactor enhancement application #3

Merged

Conversation

mraspaud
Copy link

@mraspaud mraspaud commented Aug 5, 2022

  • Closes #xxxx
  • Tests added
  • Fully documented
  • Add your name to AUTHORS.md if not there already

@mraspaud mraspaud requested a review from djhoese as a code owner August 5, 2022 13:57
Copy link
Owner

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I think it is a step in the right direction. However, I don't see this as cleaner code even if each of the functions has fewer arguments. You've now used partial in a lot of places and it makes the code, especially for newcomers, pretty difficult to read. I realize most of that is due to the complexity of the argument binding that was being taken advantage of before but I wouldn't say this is overall cleaner.

else:
band_data_arr = func(band_data_arr, **extra_kwargs)
return band_data_arr
def on_separate_bands(func):
Copy link
Owner

Choose a reason for hiding this comment

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

Does the order of this decorator with the other dask decorators matter? If so, can it be documented here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it does, since on_dask_array for example make the decorated function act on a dask array as the docstring says.
I could write in the on_separate_bands docstring that it acts on DataArrays, ok?

Copy link
Owner

Choose a reason for hiding this comment

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

That would help, but you could even go as far as saying "If this is used in combination with on_dask_array...., then it should be used after those decorators. For example:" and then show the combination of the two decorators. Just thinking about the emails I'm going to have to answer when someone gets it wrong.

Copy link
Author

Choose a reason for hiding this comment

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

done

@mraspaud
Copy link
Author

mraspaud commented Aug 5, 2022

Thanks for putting this together. I think it is a step in the right direction. However, I don't see this as cleaner code even if each of the functions has fewer arguments. You've now used partial in a lot of places and it makes the code, especially for newcomers, pretty difficult to read. I realize most of that is due to the complexity of the argument binding that was being taken advantage of before but I wouldn't say this is overall cleaner.

I agree that the usage of partial isn't so easy to read, but I would argue that having functions inside functions, using out of scope variable isn't the best either.
Anyhow, as mentionned on slack, I propose turning apply_enhancement into a decorator, which will then let the calling function pass more argument and remove the need for partial function. I'll get to it.

@djhoese
Copy link
Owner

djhoese commented Aug 5, 2022

Awesome! Thank you!

@djhoese
Copy link
Owner

djhoese commented Aug 5, 2022

I notice linting is failing, I think this branch (mine and now yours) need to be merged with upstream main.

@mraspaud
Copy link
Author

mraspaud commented Aug 5, 2022

I notice linting is failing, I think this branch (mine and now yours) need to be merged with upstream main.

I can do that

@mraspaud
Copy link
Author

mraspaud commented Aug 5, 2022

Merging main did add a bunch of commits. Maybe I should revert and let you do that in your PR?

@djhoese
Copy link
Owner

djhoese commented Aug 5, 2022

@mraspaud Yeah I'm ok with that.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2804455664

  • 1228 of 1233 (99.59%) changed or added relevant lines in 41 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 94.61%

Changes Missing Coverage Covered Lines Changed/Added Lines %
satpy/modifiers/parallax.py 117 118 99.15%
satpy/tests/enhancement_tests/test_enhancements.py 30 31 96.77%
satpy/tests/modifier_tests/test_parallax.py 347 348 99.71%
satpy/composites/init.py 51 53 96.23%
Files with Coverage Reduction New Missed Lines %
satpy/resample.py 2 79.56%
Totals Coverage Status
Change from base Build 2562551102: 0.1%
Covered Lines: 41532
Relevant Lines: 43898

💛 - Coveralls

@mraspaud
Copy link
Author

mraspaud commented Aug 5, 2022

Done.

@djhoese
Copy link
Owner

djhoese commented Aug 5, 2022

This is pretty nifty 😉

Let's merge it and see what happens. Thanks for rewriting all of this. It really feels like it puts the "control" back into the individual enhancement functions.

@djhoese djhoese merged commit 0fb7080 into djhoese:feature-enh-map-blocks Aug 5, 2022
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.

3 participants