Skip to content
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

2 adding the downloader #9

Merged
merged 11 commits into from
Jul 29, 2024
Merged

2 adding the downloader #9

merged 11 commits into from
Jul 29, 2024

Conversation

f-PLT
Copy link
Collaborator

@f-PLT f-PLT commented Jun 12, 2024

Creating this PR as a draft since while the downloader itself is contained, the structure I'm putting in place could affect the work of @mjfortier and @liellnima.

I have part of the CI passing, as Pylint is far from fixed (this is what happens when you don't use it from the start hehee!)

Right now, it's mostly the code as is, + some quick and easy fixed to make the different checks stop crying, and haven't made a test download yet (coming soon!)

So, yeah, this is so we make sure not to spread in 3 different package structures and put the things in the same place.

For example, I created only utils.py, as I'm not sure we'll have use for many different kinds of utility functions for now, and maintained the Google Docstring format, which is already in use (though we need to rename params: to Args:) For more info in Google Docstrings : https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html)

I'll surely have questions, but nothing urgent.

Closes #2

@f-PLT f-PLT requested review from mjfortier and liellnima June 12, 2024 19:55
@f-PLT f-PLT self-assigned this Jun 12, 2024
@f-PLT f-PLT linked an issue Jun 12, 2024 that may be closed by this pull request
15 tasks
Comment on lines +7 to +10
RAW_DATA = DATA_DIR / "raw"
PROCESSED_DATA = DATA_DIR / "processed"
LOAD_DATA = DATA_DIR / "load"
META_DATA = DATA_DIR / "meta"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick remark : there used to be a file called data_paths.py I just ported the important paths here instead with the other Path constants I already had

Copy link
Collaborator

@mjfortier mjfortier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the linter is failing but otherwise this looks good to me.

@f-PLT
Copy link
Collaborator Author

f-PLT commented Jul 29, 2024

Yes, Pylint fails for now as making it pass will require much refactoring (which would be futile considering there's still a lot of refactoring to do)

I added TODOs to the code to document things that still need to be done, but the structure is far enough along for a first merge.

Things that need to be done :

  • downloader.download_meta_historic_biomassburning_single_var : first draft of refactor, still needs to be tested
  • downloader.download_from_model_single_var : Not refactored yet and not tested.
  • General refactor for efficiency and maintainability. Still have to have the discussion on how we want to manage the data and when to process it.

@f-PLT f-PLT marked this pull request as ready for review July 29, 2024 19:58
@f-PLT f-PLT merged commit 514e9a6 into main Jul 29, 2024
3 of 4 checks passed
@f-PLT f-PLT deleted the 2-adding-the-downloader branch July 29, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding the Downloader
2 participants