Conversation
VeckoTheGecko
requested changes
May 1, 2025
Collaborator
VeckoTheGecko
left a comment
VeckoTheGecko
left a comment
There was a problem hiding this comment.
Just a small change so that we aren't comparing against a repr :)
src/virtualship/cli/_fetch.py
Outdated
|
|
||
| if "DRIFTER" in instruments_in_schedule: | ||
| print("Drifter data will be downloaded") | ||
| if "DRIFTER" in str(instruments_in_schedule): |
Collaborator
There was a problem hiding this comment.
Just wondering what the motivation for this is? Was it not working before?
Since instruments_in_schedule is a call to schedule.get_instruments() giving a set[InstrumentType], I think the right way to do this would be
Suggested change
| if "DRIFTER" in str(instruments_in_schedule): | |
| if InstrumentType.DRIFTER in instruments_in_schedule: |
(also making sure InstrumentType is imported at the top)
Collaborator
Author
There was a problem hiding this comment.
Yes, much more sensible. I've made these changes. I've added the InstrumentType import within the _fetch function to avoid circular import errors. Does that seem sensible?
Collaborator
There was a problem hiding this comment.
Yes, that sounds good. I'm not sure why we're getting circular imports, might be worth me looking at the package structure.
But yes, a lot of code sometimes puts the import in the function body rather than the top of the file
j-atkins
added a commit
that referenced
this pull request
May 15, 2025
* update init command to use CTD_BGC as new instrument when making schedule * update example ship_config file to also include CTD_BGC instrument config * Prevent fetch from downloading unnecessary data and skipping data download for certain instruments (#175) * fixes for over-extensive data downloads and skipping certain instruments * fixes based on PR feedback #175 * Managing conflicts on branch. Move InstrumentType to ship_config.py * Move Waypoint to schedule.py * update init command to use CTD_BGC as new instrument when making schedule * add configuration for CTD_BGC * add bgc data download option for CTD_BGC instrument * add CTD_BGC to instruments which prompts ship data download * Review feedback on complete_download --------- Co-authored-by: Vecko <36369090+VeckoTheGecko@users.noreply.github.com>
j-atkins
added a commit
that referenced
this pull request
May 15, 2025
* update init command to use CTD_BGC as new instrument when making schedule * update example ship_config file to also include CTD_BGC instrument config * Prevent fetch from downloading unnecessary data and skipping data download for certain instruments (#175) * fixes for over-extensive data downloads and skipping certain instruments * fixes based on PR feedback #175 * Managing conflicts on branch. Move InstrumentType to ship_config.py * Move Waypoint to schedule.py * update init command to use CTD_BGC as new instrument when making schedule * add configuration for CTD_BGC * add bgc data download option for CTD_BGC instrument * add CTD_BGC to instruments which prompts ship data download * Review feedback on complete_download * bgc input data .nc files for testing * add CTD_BGC to static/example schedule (also necessary for ship_config test) * update test suite for new bgc sampling using ctd_bgc * new ctd_bgc instrument * add ctd_bgc sampling capability to virtualship run execution --------- Co-authored-by: Vecko <36369090+VeckoTheGecko@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #174