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

Upadate of transit spectra calculator #57

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Upadate of transit spectra calculator #57

wants to merge 14 commits into from

Conversation

Kiefersv
Copy link
Collaborator

Hey all,

I updated the transit spectrum calculator. Instead of using a fixed longitudinal opening angle for all latitudes, it now uses a coordinate transformation to average over the full terminator grid cell, even a the pols.

I still need to adjust the tests, but the code is arleady tested and should work.

@Kiefersv
Copy link
Collaborator Author

I checked the tests and I am not sure if I broke the unit tests or not because to me they seem unrelated to my changes. @AaronDavidSchneider could you have a look at it?

@AaronDavidSchneider
Copy link
Contributor

I raised this as an issue in the h5py repo. Lets see if they reply.

@AaronDavidSchneider
Copy link
Contributor

AaronDavidSchneider commented Oct 24, 2023

I tried to fix it in this commit:
8fc113a

Edit: not working yet: I will fix it in the next days in #58

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #57 (cd24e5f) into main (d462282) will increase coverage by 0.34%.
The diff coverage is 91.22%.

❗ Current head cd24e5f differs from pull request most recent head 4367309. Consider uploading reports for the commit 4367309 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   78.30%   78.65%   +0.34%     
==========================================
  Files          14       14              
  Lines        1549     1593      +44     
==========================================
+ Hits         1213     1253      +40     
- Misses        336      340       +4     
Flag Coverage Δ
unittests 78.65% <91.22%> (+0.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
gcm_toolkit/utils/interface.py 50.51% <91.22%> (+3.26%) ⬆️

@Kiefersv
Copy link
Collaborator Author

Got it to work! Thanks again @AaronDavidSchneider for your help with the h5py problems.

Copy link
Contributor

@AaronDavidSchneider AaronDavidSchneider left a comment

Choose a reason for hiding this comment

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

looks good to me! Thanks @Kiefersv

Copy link
Contributor

Choose a reason for hiding this comment

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

in an ideal world, we would add these hardcoded values in the test to the test configuration in test_gcmtools_common.py

Copy link
Contributor

Choose a reason for hiding this comment

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

... but I am also fine keeping it like this.

@Kiefersv
Copy link
Collaborator Author

There is something wired in the test values, i will have closer look

@AaronDavidSchneider
Copy link
Contributor

any update @Kiefersv?

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.

2 participants