ArtesiaWater / hydropandas

Module for loading observation data into custom DataFrames
https://hydropandas.readthedocs.io
MIT License
51 stars 11 forks source link

restructuring of custom functions #1

Closed dbrakenhoff closed 4 years ago

dbrakenhoff commented 5 years ago

Wondering out loud whether it would make sense to move some of our custom functionality to a submodule, i.e. moving all the custom plotting capability to ObsCollection.customplots or something?

Because of the large amount of methods and attributes in pandas dataframes it's sometimes hard to find our custom methods without looking at the source code. Restructuring this way might be helpful in that regard. But it might also introduce some difficulties that I haven't yet thought about.

Curious about your thoughts!

OnnoEbbens commented 5 years ago

Sounds like a good plan to me. Just wondering if this means we should create a submodule for the Obs classes as well. I guess we can start with the ObsCollection class.

dbrakenhoff commented 4 years ago

I did a proposal in the restructure branch: aa43df41d4d5d90b8147d452ffb8e549e141e1ed

I wasn't sure how to add in a class in ObsCollection and make it persist after slicing etc. So I found a way of adding namespaces with different functionality by registering an accessor (see here). Perhaps this can all be done simpler, but this seems to work.

I created 4 categories of functions

These are now all accessible from an ObsCollection object with e.g.

oc = ObsCollection(...)
oc.stats.find_min_max(...)

The tests all pass, but I might still have missed a few renames when moving the methods that aren't hit by the tests.

What do you think?

OnnoEbbens commented 4 years ago

Nice! I think this is the perfect solution.

I am making an attempt to reorganise the directory structure a bit (create a new directory for the io modules). maybe you can wait for this before pushing to dev.

dbrakenhoff commented 4 years ago

Makes sense, I'll wait for your reorganization before submitting a PR. It shouldn't have too much overlap though, I only really touched the ObsCollection and Obs objects. So merging should be ok.

I just did the same for the Obs class, though the number of methods there is a lot lower, so not entirely sure if it's necessary. But for consistency it's probably a good idea to do it for Obs as well.

OnnoEbbens commented 4 years ago

Good idea to do this for the Obs class. Nice how the methods for the ObsCollection are seperated from the Obs methods!

I moved the io modules to a new directory. All the test, except for the Dino download functions, pass. So we can merge this into dev I guess.

OnnoEbbens commented 4 years ago

merged to dev, closing this issue