search

AI-multimodal / aimmdb

BSD 3-Clause "New" or "Revised" License
0 stars 10 forks source link

Added larch normalization scheme to postprocessing #48

Closed CharlesC30 closed 1 year ago

CharlesC30 commented 1 year ago

Created new class for larch normalization scheme in operations module. Also added testing for new operator in tutorial notebook.

CharlesC30 commented 1 year ago

@x94carbone

CharlesC30 commented 1 year ago

@x94carbone I added an explanation of the normalization process in larch to the NormalizeLarch docstring. Some of it is copied from the larch source code then modified slightly for how we are using it here.

CharlesC30 commented 1 year ago

Also if we wanted to we could add more parameters to this class to interface with larch more. For example there could be a keyword argument e0 (default equal to None), that is also given to the larch pre_edge function during normalization. This could also be done for other keyword arguments in the pre_edge function. Though I am not sure if this is how we want these operators to work.

matthewcarbone commented 1 year ago

@CharlesC30 perhaps just add a **kwargs arg to the init or something like that?

CharlesC30 commented 1 year ago

@CharlesC30 perhaps just add a **kwargs arg to the init or something like that?

good idea! added

CharlesC30 commented 1 year ago

I like this. That would be part of forcing the Larch normalization parameters, right?

@x94carbone yeah exactly! By default larch calculates most parameters itself but we could explicitly pass things that are already known.

Give it some thought, let me know what you think! The general idea is that the operator would take these types of parameters during instantiation. Then the __call__ method just takes what's being operated on.

Okay I think it's not a bad idea then. I was looking at the other operators though and think maybe we should use the same approach as the StandardizeGrid operator. Basically provide a dict keyword argument to the __init__ that contains all the larch pre_edge kwargs, then link to the pre_edge documentation in our docstring.

matthewcarbone commented 1 year ago

@CharlesC30 Yep this is really good, great work, thank you! Basically exactly what I was looking for. I think this PR is good to go unless you have any other suggestions.

CharlesC30 commented 1 year ago

@x94carbone All good with me! I think it's okay to merge