-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
refactor: Reorganized references folder #104
refactor: Reorganized references folder #104
Conversation
Codecov Report
@@ Coverage Diff @@
## master #104 +/- ##
=======================================
Coverage 92.33% 92.33%
=======================================
Files 18 18
Lines 652 652
=======================================
Hits 602 602
Misses 50 50
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
Thanks for the PR! While I agree the priority should be set to WildFire, considering the dataset version we use for training is not yet available publicly in the lib, I suggest keeping the OpenFire training by merging the two scripts (having an option pointing to the dataset the user wants to train on, wildfire/lightfire by default). What do you think?
ok I understand your point I'll make the change |
Hi fg, I added the possibility to train a model with openfire. We need to make Wildfire downloadable in the same way (with a flag download=True) in a futur PR. |
There is an issue with doc build, there is already an open issue on the repo : sphinx-doc/sphinx#8885 |
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 last change and we're good to go!
Watch out not to mix PR purposes together, I just noticed that you removed utils.rst in the documentation. This should be done in a separate PR as this is a completely different topic, I'll open one to handle this quickly 👌 Best practices:
Otherwise it gets very hard quickly to track back past modifications :/ |
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.
All good, I think you merged unwanted modifications as well (cf. comments). But it's really nitpicking
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.
Sorry my previous feedback wasn't clear apparently 😅
Also, I didn't realize, but you have to update the instructions of the "References" section in the main README ;) |
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.
One last typo, and we can merge 😅
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 good to me! Seems like you'll need to merge master before moving on though
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.
Thanks for the edits!
The purpose of this PR is to close #103