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.

Sign up for GitHub

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

Jump to bottom

First try at fixing col setup without start stop. Model validator for… #1212

Open
wants to merge 2 commits into
base: main-dev
Choose a base branch
from
Open

First try at fixing col setup without start stop. Model validator for… #1212

wants to merge 2 commits into from
+25 −8

Conversation

dulte
Copy link
Collaborator

@dulte dulte commented Jun 18, 2024 •
edited
Loading

… pydating is also removed

Change Summary

Changes how the colocation setup and colocator handles start and stop times

Related issue number

Fix #1211

Checklist

  • Start with a draft-PR
  • The PR title is a good summary of the changes
  • PR is set to AeroTools and a tentative milestone
  • Documentation reflects the changes where applicable
  • Tests for the changes exist where applicable
  • Tests pass locally
  • Tests pass on CI
  • At least 1 reviewer is selected
  • Make PR ready to review

@dulte dulte added the bug 🐛 Something isn't working label Jun 18, 2024
@dulte dulte added this to the m2024-07 milestone Jun 18, 2024
@dulte dulte requested a review from lewisblake June 18, 2024 09:19
lewisblake
lewisblake reviewed Jun 18, 2024
View reviewed changes
pyaerocom/colocation/colocation_setup.py Outdated
raise ValueError("Both start and stop need to be provided or both not provided.")
return self

# @model_validator(mode="after")
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just leave this in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can. But then we have to remove the return self above. I'm not sure if this might lead to other strange behavior

@dulte
Copy link
Collaborator Author

dulte commented Jun 18, 2024

Some test do not work due to

            if self.colocation_setup.stop is not None or not isinstance(
                self.colocation_setup.start, int
            ):
                raise ColocationSetupError(
                    "Conflict: only single year analyses are support for model "
                    'climatology fields, please specify "start" as integer '
                    'denoting the year, and set "stop"=None'
                )

part of the colocator code

@dulte dulte mentioned this pull request Jun 18, 2024
Open
@lewisblake lewisblake modified the milestones: m2024-07, m2024-09 Jul 1, 2024
@lewisblake
Copy link
Member

@dulte pushing this back until September to match the milestone for the associated PR. This is the type of issue I think we should clarify at the AeroTools backend design retreat, so please keep in mind as an example.

avaldebe
avaldebe requested changes Aug 1, 2024
View reviewed changes
Copy link
Collaborator

@avaldebe avaldebe 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 a few details that can be improved.
Also, not that start and stop need to be present I would appreciate tests when one of them is missing in order to ensure that an exception is raised.

pyaerocom/colocation/colocation_setup.py
if not p.exists():
p.mkdir(parents=True, exist_ok=True)
return str(p)
return self
Copy link
Collaborator

Choose a reason for hiding this comment

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

why return self?

pyaerocom/colocation/colocation_setup.py
if self.start or self.stop:
raise ValueError("Both start and stop need to be provided or both not provided.")

# return self
Copy link
Collaborator

Choose a reason for hiding this comment

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

the previous validator returns `self, why this one does not?

pyaerocom/colocation/colocation_setup.py
Comment on lines +472 to +473
if not (self.start and self.stop):
if self.start or self.stop:
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the nested if needed?
maybe this could be re-written as

        if not (self.start and self.stop) and self.start or self.stop:

or

        if bool(self.start) != bool(self.stop):

pyaerocom/colocation/colocation_setup.py
Comment on lines +502 to +503
if not p.exists():
p.mkdir(parents=True, exist_ok=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see no need for the if, the exist_ok=True parameter does the checking.
You can simple write:

        p.mkdir(parents=True, exist_ok=True)

@heikoklein heikoklein modified the milestones: m2024-09, m2024-10 Sep 11, 2024
@lewisblake lewisblake modified the milestones: m2024-10, m2024-11 Oct 7, 2024
@lewisblake lewisblake modified the milestones: m2024-11, m2024-12 Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lewisblake lewisblake lewisblake left review comments

@avaldebe avaldebe avaldebe requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@dulte dulte

@lewisblake lewisblake

Labels
bug 🐛 Something isn't working
Projects
None yet
Milestone
m2024-12
Development

Successfully merging this pull request may close these issues.

Empty colocated object when start and stop are not given
4 participants
@dulte @lewisblake @avaldebe @heikoklein