search

gogulan-k / FID-Net

Deep Neural Networks for Analysing NMR time domain data
MIT License
10 stars 6 forks source link

Refactor into installable package #1

Closed jorenretel closed 7 months ago

jorenretel commented 8 months ago

Hi Gogulan,

I recently saw a talk by Prof. D. Flemming Hansen talking about FID-Net and I was really impressed. I think this project is great and I appreciate the effort you've put into it. The outlook that we could be designing new NMR experiments that will really go hand in hand with the deep learning methods to post-process the data is really cool.

Last week, I had the opportunity to try everything out using the example data. Everything ran smoothly after making some minor adjustments (because I did not run the exact same version of tensorflow and there were some imports that were not found. But luckily these were exactly imports that were actually not used in the script).

I ended up re-organizing the code a bit so it is easier for our internal use. Instead of keeping these changes internal I thought it would be a good idea to share with you what I did. I believe these changes can improve the user experience for others who are trying out FID-Net. However, I also understand when you would like to keep the code as it is.

The main things that I did:

  1. refactor the code into a pip installable pyton package by adding a setup.cfg and pyproject.toml.
  2. this allows the CLI (that I ported to Typer) to installed and the commands to be found from wherever you are in th file system. It prevents having to put the path to the individual scrips manually on your $PATH if you want to run the command from somewhere else. Typer is nice because the documentation and help messages are really useful.
  3. Added some code to download the weights and example data automatically. Github has a "release" feature that allows you to upload files to a release (not lfs). Since the weights and example files are not very large, they will fit there. When you agree and we adjust the URL in _config.py to point to that data, the weights (and example data) can be downloaded automatically. This was a change I made because the proxy that I am behind is blocking dropbox.

I hope you find some of these changes useful. In general, I did not change anything about the actual functionality of the code. If you consider merging this PR but I renamed something wrong while moving things around or in general misinterpreted something, please let me know and I will correct it.

Again, this is awesome work and I am looking forward to seeing what you and your team will do next.

Best regards,

Joren Retel

gogulan-k commented 7 months ago

Hi Joren,

Thank you so much for your message and apologies for the slow repsonse. I'm really glad to hear that you enjoyed hearing about FID-Net and tried out the examples. I'm even more grateful for your feedback and for submitting your pull request.

The changes you've suggested sound great and we are always looking for ways to improve access to these tools for the NMR community. Right now we have a paper under review so will wait to merge these changes until we have a response on that and in the meantime I'll test out the changes and let you know if I have any questions. But hopefully before too long can merge this and credit you for your contribution.

Best, Gogs

jorenretel commented 7 months ago

Hi Gogs,

no worries. And thanks for your response. Sounds good to me. If you have any problems getting my branch to run, please reach out, because the whole point is that it should make things easy. I added a description of what to do in the README. Mind that the automatic download of the weights and example files does not work yet, because it depends on them being uploaded to a GitHub release (see point 3. in my initial comment). But putting them in a "data" directory in the repo root like this will prevent the download from being necessary:

├── example
│   ├── ca_detect
│   │   ├── decouple.ft1
│   │   ├── decouple.ft2
│   │   ├── fin_proc.com
│   │   └── test.ft1
│   ├── con_decouple
│   │   ├── decouple.ft1
│   │   ├── decouple.ft2
│   │   ├── final_proc.com
│   │   └── test001.ft1
│   ├── ctcp_decouple
│   │   ├── decouple.ft1
│   │   ├── decouple.ft2
│   │   ├── final_proc.com
│   │   └── test001.ft1
│   ├── hnca_decouple
│   │   ├── final_proc.com
│   │   ├── T4L_example.md
│   │   └── test.ft2
│   ├── methyl
│   │   └── test.fid
│   └── nus
│       ├── hdac_ex.ft1
│       └── ss_hdac.out
└── weights
    ├── fidnet_13c_methyl.h5
    ├── fidnet_1h_methyl.h5
    ├── fidnet_recon.h5
    ├── jcoup_2dcadet.h5
    ├── jcoup_2dcon.h5
    └── jcoup_2dctcp.h5

If you want I can upload them to a release on my forked repo and adjust the code so the automatic download works. But I was not sure whether you would be okay with it when I brought the weights from dropbox to GitHub and therefore wanted your permission.