-
Notifications
You must be signed in to change notification settings - Fork 11
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
The Big Merge #30
The Big Merge #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Scott
So, I finally managed to get through it.
First of, this bears repeating from slack.
Just in case I forget to mention it in the review. With one teeny tiny exceptions, which I don't really know yet how to solve, the figure code reads like an absolute joy! :star-struck: . It reads really clean and the compos-ability brought in by the ability to pass around axis, makes the code for all the composite/(more specific) figures really nice, easy to read and without copied code and I think the few cases of factoring common procedures out (making panels and such) are just the right amount and with interface at the right abstraction level. It is incredibly good!
The one teeny-tiny thing is that plotters consequently take a default measurement and a measurement override in the plotting methods. At first I thought it was an anti-pattern (violating the "There should be one-- and preferably only one --obvious way to do it." and so I commented on it, until I realized that it seemed to actually be used and that I didn't have a good suggestion for how to do it otherwise. So when you get to those comments, just ignore them for now, except then one where I think there is a semantic problem, because we may be referencing a measurement that doesn't exist. The review is unfortunately way to big for me to find them again and delete them :(
With regards to your focus point, I didn't notice anything worth commenting on for the first three. It looks good to me. With regards to general readability I think it is good. There are a few minor exceptions, but nothing that scares me:
- Maybe because we come from academia (or maybe matlab) there is a tendency in come of the code to abbreviate more than I prefer. In these days where everyone and their aunt has a auto-completing editor, I would much rather have pretty much everything written out and improve the decipherability in the process. If it is for economy of typing while writing the code, I would suggest to just search a replace when done.
- There are a few places here where relatively complex formulae or (vectorized) np formulae is just there, without any explanation or reference. That may be ok, if this is just the kind of formulae that people within the field has tattooed to their body, or as long as the author is still active, but it can be a maintainability problem in the long wrong.
- Finally. I think there is some more things to work with and possible iterate on with regards to composition, but that can be for another time.
All in all, this looks fantastic and I can't wait to get it merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update periodically as I implement the review. This first one is everything outside of "src", i.e. mainly docs/ and development_scripts/
Most issues were easy-to-resolve typos (amazed how much you catch!). Unresolved comments mainly relate to the purpose of development_scripts. Let me know if you accept my explanation, and whether they are stashed in the right place for that purpose.
@ScottSoren I went through all unresolved in everything but src and left just a single unresolved, which is the Jupyter/IPhython comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kenneth, here's the next chunk of my implementation of the review, covering Exporters.
The one bit of pending implementation is removing the ability to pass a measurement into the export
method. To do so, I could use your input (again) on the fundamental design discussion here:
https://github.com/ixdat/ixdat/pull/30/files#r810926968
I've completed your last implement. I suggest as for the export and plotters to leave it as is, since it works and is at worst an API annoyance, if it even is that. In any case, if we defer we can get the Big Merge in and maybe to small quick draft experiments to explore design options for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review of measurment.py, plotters/, readers/, and spectra.py here. Just techniques/* to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! Done implementing :D
Ready to merge, awaiting your approval.
Ready to merge when you change the status of the review from "changes requested". |
New contributions that were made to the user_ready branch while we were working on #30
Here it is, the big PR.
This contains user-ready tools for analyzing data from
It also contains the documentation viewable at https://ixdat.readthedocs.org
It has been tested against:
It has not yet been tested against other tutorials and article repositories, but I imagine that will not result in huge changes for ixdat.
I suggest viewing source modules in the following order:
Challenges to focus on include:
Note that while spectroelectrochemistry.py is not a priority here (Caiwu gave the module as well as the corresponding plotter and readers a thorough review in #13), spectra.py is important with respect to coming implementation of mass scan analysis.
I think the docs to be out of scope for a thorough review here, but the documentation does need improvement!
There are TODOs and FIXMEs spread throughout the code. Most of them have to do with a coming "metaprogramming" piece of work which will fix the class attributes of Saveable classes to serve as database design blueprint, and simplify parts of the code.
Looking forward to being back on master for distribution, docs, and everything else :D