-
Notifications
You must be signed in to change notification settings - Fork 7
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
Initial support for loading NLD/GSF from disk #139
base: master
Are you sure you want to change the base?
Conversation
There seems to be some confusion: There was a Lines 34 to 35 in 0fba72a
The cleanest alternative would be to refactor the code to have a clear |
Two files was added by accident!
|
I've added a dedicated save method and refactored the `extract_from()` method
Refactored. There should also be written a unit test for the methods. Will do it before pull. |
Guess this is why you want unit tests...
I know... I know... Btw, does anyone read these?
I've updated the getting started notebook to reflect the change in syntax.
I think that before merging this PR there should be a super class defining the save/load syntax. Probably something like class LoadSaver():
def __init__(self, path: Union[str, Path]):
if path is not None:
self.path = Path(path)
try:
self.load(self.path)
except ValueError:
self.path.mkdir(exist_ok=True, parents=True)
def load(self, path: Optional[Union[str, Path]] = None):
raise NotImplementedError
def save(self, path: Optional[Union[str, Path]] = None):
raise NotImplementedError and require the subclasses to implement |
Thanks for implementing the abstract loadsaver. I guess we should then make Extractor (and the normalizers) subclasses of |
c9243d2
to
c1aab18
Compare
I've now made Extractor a subclass of the abstract_load_saver: The ensemble class will require some refactoring, while the normalizers already has something similar built in the I'm not sure I want to do that much refactoring... Need to do prioritize the actual analysis I'm working on :/ |
I don't think I will be working much more on this PR anytime soon. Maybe just merge and then create an issue noting that other classes needs refactoring to use |
I've added a static method to the
Extractor
class that will allow the user to load an existing ensemble of NLD and gSF from disk. This will eventually close issue #138.At the moment there are some issues that will need to be resolved before ready for merging: