LM-SAL / aiapy

Python library for AIA data analysis
https://aiapy.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

General Tidy up of docs and fixes to some warning types - [merged] #294

Closed nabobalis closed 8 months ago

nabobalis commented 1 year ago

Merges register -> main

All changes that this PR does are documented in the changelog.

Other misc changes have no user facing changes but tidy up RST and CI issues.

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

marked this merge request as ready

nabobalis commented 1 year ago

added 3 commits

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 12, 2023, 11:11

Commented on aiapy/calibrate/prep.py line 97

I think I am :thumbsdown: on this approach. Is there a particular reason that level 1.5 maps must always have dimensions (4096,4096)? Is this just to maintain consistency with what is done in IDL? (that is fine if that is the case).

If so, I think instead of special casing some particular shape, we should have some sort of cropping/padding operations that ensures that the centered, de-rotated, and scaled image always has a width and height of 4096 pixels.

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 12, 2023, 11:15

Commented on aiapy/calibrate/prep.py line 82

(This does not really relate to the changes made in this PR)

I don't think this if-else block is needed. The target scale should always be 0.6 "/pixel

    scale = 0.6 * u.arcsec

This function should also only be applied to level 1 maps (which by definition have not been rescaled).

nabobalis commented 1 year ago

I actually don't know what the IDL version does. I should check.

Is it possible to do that generally to ensure the image is always 4096?

nabobalis commented 1 year ago

I will remove it.

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 12, 2023, 12:36

Commented on aiapy/calibrate/prep.py line 97

Ha well with my proposed padding approach in submap it will be :stuck_out_tongue:. Otherwise, no, I don't think so (not without reprojection at least).

nabobalis commented 1 year ago

Ok, so I will remove this then. Add a note in the doc string or something.

nabobalis commented 1 year ago

changed this line in version 8 of the diff

nabobalis commented 1 year ago

changed this line in version 8 of the diff

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

Now removed.

nabobalis commented 1 year ago

Removed, added a warning to the docstring.

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 13, 2023, 06:56

Sorry, but the scope of this PR has now become unclear to me and I'm having a hard time reviewing it. Could you move all the register-related commits to a separate PR? Or if it is easier, just remove them. If the latter, I can open a separate PR to deal with my suggested changes.

I think it is important to keep any changes to register in their own PRs and not buried amongst other changes, given its importance in the "prep" phase of analyzing AIA data.

nabobalis commented 1 year ago

This PR now does 0 actual changes to register.

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

I updated the changelog to say that this PR does.

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

This has been reverted now.

nabobalis commented 1 year ago

resolved all threads

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 17, 2023, 11:52

Commented on aiapy/calibrate/tests/test_prep.py line 37

I would keep this comment. It links to the issue that explains why this is not checking for 4096

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 17, 2023, 11:55

approved this merge request

nabobalis commented 1 year ago

changed this line in version 18 of the diff

nabobalis commented 1 year ago

added 1 commit

Compare with previous version

nabobalis commented 1 year ago

In GitLab by @wtbarnes on Jan 17, 2023, 11:55

resolved all threads

nabobalis commented 1 year ago

mentioned in commit 8a23c715b552d0024f5421b3d76152c00a50c9ba