OpenSenseAction / mergeplg

Merging methods for rainfall sensor data provided as point, line and grid
BSD 3-Clause "New" or "Revised" License
0 stars 4 forks source link

Rewrite highlevel-classes #21

Closed eoydvin closed 1 month ago

eoydvin commented 1 month ago

This PR implements some of the major changes discussed at workshop in Norrköping. Generally:

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 98.53480% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@fa3e6f7). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/mergeplg/merge.py 97.40% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #21 +/- ## ======================================= Coverage ? 95.66% ======================================= Files ? 10 Lines ? 553 Branches ? 0 ======================================= Hits ? 529 Misses ? 24 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cchwala commented 1 month ago

Thanks for the update! I have only limited availability this week, but will try to have a look.

Is the current state meant to be ready to merge, or do you need to do some changes still?

eoydvin commented 1 month ago

I will have a look at variable naming and overall code documentation tomorrow, so the code is not ready to merge yet.

eoydvin commented 1 month ago

@cchwala I included the automatic variogram estimation in this PR as it made writing tests simpler. I think the code is ready for merging, so if you cold have a look that would be good.

cchwala commented 1 month ago

I have not had a look at all details, but here are two points

  1. I suggest to only plot some selected time steps since the notebook should in the end also provide a concise overview in the style of a documentation (note that you should add it to index.md in the docs dir). Also the notebook file size is pretty large with that many plots.
  2. On question regarding the .update() method: I still like the idea of having this option to update the required setup for adjustment. But from the notebook it looks like .update() could just always be called inside .adjust(). Is that correct and is it a good idea? Is there a use case where one would call .updated() differently from what .adjust() could do, based on the data that it receives as input.
eoydvin commented 1 month ago

It is not necessary to always run update() before adjust, so we could move it to before the for loop in the notebook. On the other hand, update() is also fast when the geometry is already known. Keeping it outside the adjust function makes it more explicit for the user, but keeping it inside could lead to the user being able to focus on more important things. I will try to move update into adjust now and then we can see.

cchwala commented 1 month ago

Thanks for the update and also thanks for the very good test coverage!

I had a quick look again and I like it! Of course, some things will need discussion and some things will need polishing, but, as written on Zulip, I am merging now so that we can procceed!