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

fixed geoloc_example and added variable descriptions #121

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

Spect4tor
Copy link

@Spect4tor Spect4tor commented Mar 8, 2023

Geoloc_example was no longer working. This was also mentioned in Issue #105. The example is now structured like in the instrument definitions, along with some extra comments to explain the meaning/origin of the variables used.

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes flake8 pyorbital
  • Fully documented

times = (np.tile(scan_points * 0.000025 + 0.0025415, [scanline_nb, 1])
+ np.expand_dims(offset, 1))
times = np.tile(scan_points * int_t, [np.int32(scans_nb), 1])
offset = np.arange(np.int32(scans_nb)) * scan_p
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to cast the scans_nb to int32?

Copy link
Author

Choose a reason for hiding this comment

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

This is based on "geoloc_instrument_definitions.py" line 65. I'm not very experienced with this package and just tried to copy the format of what is done in that file, since that worked.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I understand. But I think just in the example, the casting is probably not needed. Would you mind trying without and see if the results look correct?

Copy link
Author

Choose a reason for hiding this comment

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

I removed it and results were identical.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks a lot! then I guess this is good to go :)

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM if we remove the integer casting

times = (np.tile(scan_points * 0.000025 + 0.0025415, [scanline_nb, 1])
+ np.expand_dims(offset, 1))
times = np.tile(scan_points * int_t, [np.int32(scans_nb), 1])
offset = np.arange(np.int32(scans_nb)) * scan_p
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
offset = np.arange(np.int32(scans_nb)) * scan_p
offset = np.arange(scans_nb) * scan_p

offset = np.arange(scanline_nb) * 0.1666667
times = (np.tile(scan_points * 0.000025 + 0.0025415, [scanline_nb, 1])
+ np.expand_dims(offset, 1))
times = np.tile(scan_points * int_t, [np.int32(scans_nb), 1])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
times = np.tile(scan_points * int_t, [np.int32(scans_nb), 1])
times = np.tile(scan_points * int_t, [scans_nb, 1])

avhrr = np.vstack(((scan_points / 1023.5-1) * np.deg2rad(-scan_angle),
np.zeros((len(scan_points),))))
avhrr = np.tile(
avhrr[:, np.newaxis, :], [1, np.int32(scans_nb), 1])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
avhrr[:, np.newaxis, :], [1, np.int32(scans_nb), 1])
avhrr[:, np.newaxis, :], [1, scans_nb, 1])

@Spect4tor Spect4tor requested a review from mraspaud March 22, 2023 16:13
@mraspaud mraspaud added the bug label Mar 22, 2023
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #121 (83c1689) into main (cad4342) will decrease coverage by 0.16%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
- Coverage   87.65%   87.49%   -0.16%     
==========================================
  Files          15       15              
  Lines        2203     2207       +4     
==========================================
  Hits         1931     1931              
- Misses        272      276       +4     
Flag Coverage Δ
unittests 87.49% <0.00%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
pyorbital/geoloc_example.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mraspaud mraspaud merged commit d3e7c2f into pytroll:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants