-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add tutorial notebook for RISE on FRB timeseries data #714
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
To Do:
|
Tutorial added to README, logo will come later (see #719). |
#718 is handled in this PR as well - I considered doing it separately but when reviewing notebooks I thought it easier to just have everything in one go. |
Ah, meanwhile, I have messed up with the structure affecting mist the notebooks and their data, labels, and models in #699 (currently up to reviewing)... I wonder what order of merging is best? |
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.
Great notebook with the extra's about shifting and adding other sources. One of the coolest tutorials that we have now! Thanks for giving this some love ;-)
I made some comments about some details.
@loostrum , at the last stand-up, only myself and @cwmeijer were present and I had put a "stand-up" label for my issue (and branch) #667 (PR #699). We think, that if you have to address some comments anyway, maybe you can also apply the small changes according to the new folder structure and we merge my PR first, what do you think? I'll put soon the folder structure in "main" and in "667-reorganize" for convenience. |
Old and proposed folder structures of
|
Yes that seems like a good idea! When #699 is merged I'll check that this PR then matches the new layout |
Most review comments handled, logo added. I'm not sure what to do about the FRB dataset section. We identify a tutorial by a logo and then explain that logo by connecting it to a dataset, but for the FRB case there is no public dataset. I've left out a hyperlink for now. I can also link it to the paper where the model and its training are explained? |
Note: the Mac OS on Python 3.11 CI is failing for reasons unrelated to this PR |
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.
Small suggestions in notebook.
@elboyran could you check if your comments are now adequately addressed? Then I think this is ready for merging |
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.
Looks nice. Only some MD links checks should pass.
Also includes the FRB data itself, so this PR relates to both #707 and #710.
The FRB data and model nearly work with DIANNA timeseries as-is. The only issue is that the data needs some reshaping, as DIANNA expects data in (time, channel) format while the FRB model uses (channel, time). This is further explained in the notebook.