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

Merge CAMS2-83 code to main-dev #1128

Merged
merged 375 commits into from
Apr 23, 2024
Merged

Merge CAMS2-83 code to main-dev #1128

merged 375 commits into from
Apr 23, 2024

Conversation

heikoklein
Copy link
Member

@heikoklein heikoklein commented Apr 18, 2024

Change Summary

Merging cams-2-83-experiment-2 to main-dav

Related issue number

#1113

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

Copy link
Member

@lewisblake lewisblake left a comment

Choose a reason for hiding this comment

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

Impressive work getting this PR so far. I know it's taken a lot. Feel free to take my comments in stride. This is a very large PR and introduces a lot of changes that are specific to CAMS2_83 and so generally I think an effort to delineate parts of the code that are specific to cams2-83 would benefit future programmers reading the code. The tests I understand we are not going to worry about for now, but one day I think we should revisit them.

pyaerocom/aeroval/coldatatojson_helpers.py Outdated Show resolved Hide resolved
pyaerocom/aeroval/coldatatojson_helpers.py Outdated Show resolved Hide resolved
pyaerocom/aeroval/coldatatojson_helpers.py Outdated Show resolved Hide resolved
pyaerocom/aeroval/coldatatojson_helpers.py Show resolved Hide resolved
pyaerocom/aeroval/helpers.py Show resolved Hide resolved
pyaerocom/scripts/cams2_83/engine.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Generally methods in this file could benefit from more documentation in the form of docstrings

pyaerocom/scripts/cams2_83/evaluation.py Outdated Show resolved Hide resolved
tests/fixtures/cams2_83/config.py Outdated Show resolved Hide resolved
tests/fixtures/cams2_83/config.py Outdated Show resolved Hide resolved
@charlienegri
Copy link
Collaborator

charlienegri commented Apr 22, 2024

my latest commit breaks the webserver, see test-forecast-day here https://aeroval-test.met.no/charlien/pages/evaluation/?project=cams2-83&experiment=test-forecast-day&station=ALL&parameter=concpm10
basically, the slash in the period strings matters for many parts of the web visualization, so I will revert my last changes and leave it as it was, we can still remove the periodmod line and get filenames written with slashes, but then we will need to tell @AugustinMortier to adapt to this once/if we will deploy this code to production (filenames were written with slashes initially but not anymore now)

Copy link
Member

@lewisblake lewisblake left a comment

Choose a reason for hiding this comment

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

🌟 🦄 ✨ 🚀

@lewisblake lewisblake merged commit 20d6545 into main-dev Apr 23, 2024
6 of 8 checks passed
@lewisblake lewisblake deleted the cams2-83-main branch April 23, 2024 14:40
@charlienegri charlienegri mentioned this pull request Apr 24, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CAMS2_83 Issues related to the CAMS2_83 contract
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merging cams2-83-experiments-2 to main-dev
6 participants