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

Dev #175

Merged
merged 13 commits into from
Apr 8, 2024
Merged

Dev #175

merged 13 commits into from
Apr 8, 2024

Conversation

brey
Copy link
Collaborator

@brey brey commented Jan 22, 2024

No description provided.

if ihot == 2:
hotout = int((self.sdate - self.rdate).total_seconds() / info["params"]["core"]["dt"])
elif ihot == 1:
hotout = self.parameters["nhot_write"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should (almost) always add an else clase to an if/elif.

What if the user adds an invalid ihot value? Then hotout will not be defined and you will get a NameError which will confusing.

if instead we have:

else:
    raise ValueError("Acceptable values for `ihot` are 1 and 2, not: %s", ihot)

it will be immediately apparent what the issue is.

For the record, ideally, we should be validating the user input as soon as we get it, but to do that properly will require a major restructure so let's keep it simple for now.

if ihot == 2:
rnday_new = (self.sdate - self.rdate).total_seconds() / (3600 * 24.0) + pd.to_timedelta(
self.time_frame
).total_seconds() / (3600 * 24.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The formatting is ugly here. Either introduce new variables or maybe try some variation of this (I didn't test it):

            rnday_new = (self.sdate - self.rdate) + pd.to_timedelta(self.time_frame)
            rnday_new = rnday_new.total_seconds() / (3600 * 24.0)
            ...

@@ -1819,11 +1819,11 @@ def set_obs(self, **kwargs):
stations.index += 1
stations["gindex"] = mesh_index
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would even suppress assigning self.obs (above here line 1755: self.obs = tg_database)
otherwise it gets written to <module>_model.json

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with the new implementation tg_database (and thus self.obs) is always a filename, either originally or after using searvey,

            tg_database = os.path.join(path, "stations.json")
            tg.to_file(tg_database)
        self.obs = tg_database

brey and others added 4 commits April 6, 2024 16:58
add ncores kwarg, ability to load mesh file in xarray structure, fix a compatibility issue.
@pmav99 pmav99 force-pushed the dev branch 2 times, most recently from a3529d6 to 3a76d9b Compare April 8, 2024 18:10
@pmav99 pmav99 marked this pull request as ready for review April 8, 2024 19:32
@pmav99 pmav99 merged commit 5dc7789 into ec-jrc:master Apr 8, 2024
5 checks passed
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.

3 participants