Chore/docstrings #88
Chore/docstrings #88
Conversation
Wait, are we even using this or will this just be deleted anyway?
This function is very ugly, however it works and I don't think anyone should try to reformat this thing
This time for real
|
Poster is 1000DPI so 8MB. Maybe that's a bit chunky, we can have different resolutions and you guys decide whats best for the readme experience |
tiaguinho-code
requested review from
wikicraft,
mlederbauer,
strsamue and
kbiniek
tiaguinho-code
added
the
documentation
There was a problem hiding this comment.
Super nice work, thanks a lot Tiago! Particularly the examples are very nice to add to the dataloader docstring. I think it’s good to go, however there were two things that would be good to address before merging
(i) Poster resolution: Is there a particular reason we save the poster in multiple resolutions?
(ii) Visualizer and Classifier: Do we call these modules/python files in our workflow? If I remember correctly, we implemented all functionality of those files in our code, but do not need the file itself anymore. Could you kindly check if we “need” the Visualizer/Classifier for running the code and, if not, delete them? Just to not cause any confusion when the TAs review the code.
Thanks a lot in advance!!
| @@ -104,7 +104,9 @@ When the parameter `max_eval` is set to a high value such as 20, expect the whol | |||
|
|
|||
| # 🖼️Poster | |||
|
|
|||
| If you were not able to visit our beautiful poster at ETH Zurich on May 30th 2024, you can access our poster [here](TODO)! | |||
| If you were not able to visit our beautiful poster at ETH Zurich on May 30th 2024, you can access our poster [here](assets/Poster.pdf)! | |||
There was a problem hiding this comment.
Thanks a lot for taking care of this!
assets/Poster_100dpi.png
Outdated
There was a problem hiding this comment.
Do we need the poster in the other resolutions?
| @@ -31,6 +31,37 @@ | |||
|
|
|||
|
|
|||
| class DataLoader: | |||
| """ | |||
There was a problem hiding this comment.
Super nice very clean and explanatory docstrings!
nmrcraft/evaluation/visualizer.py
Outdated
There was a problem hiding this comment.
Do we even need the visualizer anymore or is it depreceated, since we running the visualization via scripts/visualize_results?
There was a problem hiding this comment.
If it is not needed anymore, I would suggest to delete the file for keeping a better overview
There was a problem hiding this comment.
The only plotting function we use in visualize_results are those from nmrcraft/analysis/plotting
There was a problem hiding this comment.
I'd suggest only starting to delete stuff once we're fully there with a working reproduce script and do all the deleting in a deletion PR (just in case)
nmrcraft/models/classifier.py
Outdated
There was a problem hiding this comment.
Same here, I very much appreciate the awesome overview of the module with the docstirngs and explanations, but isn’t this depreciated, as we are not calling the Classifier in scripts/training/one_target.py or multiple_targets.py anymore?
There was a problem hiding this comment.
not sure, just went all out
same here as above where I'd like to keep stuff until the very end
nmrcraft/utils/general.py
Outdated
There was a problem hiding this comment.
Very nicew, one option would be to rename this file to dataframe , dataframe_utils or something else? BUt this is completely optional
|
Could you kindly select the poster resolution that we need (@kbiniek ), after that we can merge to main. Thanks a lot in advance !! |
Adding docstrings to all functions in nmrcraft directory and also added the poster to the readme.