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

add unit detection for XRT data #6725

Merged
merged 18 commits into from
Feb 1, 2023
Merged

add unit detection for XRT data #6725

merged 18 commits into from
Feb 1, 2023

Conversation

exitflynn
Copy link
Member

PR Description

XRT data is now given the unit property which is DN by default and DN/sec if it's been renormalized. Addresses and closes #6716

@exitflynn exitflynn requested a review from a team as a code owner January 24, 2023 14:47
@exitflynn
Copy link
Member Author

exitflynn commented Jan 24, 2023 •
edited
Loading

Is an outcome like this what we're aiming for?
image
Looks fine to me just that the unit doesn't show up among other properties... Any pointers?

@nabobalis
Copy link
Contributor

We should add the reference information as to how we know what the unit is in the unit code.

@nabobalis nabobalis added the map Affects the map submodule label Jan 24, 2023
@exitflynn
Copy link
Member Author

does the message work? The XRT Analysis Guide is already (indirectly) linked to in the main doc string

@nabobalis
Copy link
Contributor

Reads fine to me. It will need a bugfix changelog.

@wtbarnes wtbarnes mentioned this pull request Jan 25, 2023
Merged
wtbarnes
wtbarnes requested changes Jan 25, 2023
View reviewed changes
Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

This will have to wait on #6728. This should also get a test for the unit property.

sunpy/map/sources/hinode.py Outdated Show resolved Hide resolved
sunpy/map/sources/hinode.py Outdated Show resolved Hide resolved
sunpy/map/sources/hinode.py Outdated Show resolved Hide resolved
sunpy/map/sources/hinode.py Outdated Show resolved Hide resolved
exitflynn and others added 4 commits January 27, 2023 03:11
@exitflynn @wtbarnes
Update sunpy/map/sources/hinode.py
7fc50b0 
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn @wtbarnes
Update sunpy/map/sources/hinode.py
d5966f0 
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn @wtbarnes
Update sunpy/map/sources/hinode.py
2e49164 
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn @wtbarnes
Update sunpy/map/sources/hinode.py
ea8ddd8 
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn
Copy link
Member Author

i've made the changes, should I get to writing a test for it now?

@nabobalis
Copy link
Contributor

Yes please.

@exitflynn
Copy link
Member Author

there's currently only a header file in the examples, can you link to one?

@nabobalis
Copy link
Contributor

there's currently only a header file in the examples, can you link to one?

You mean to a file that has the correct history text?

@exitflynn
Copy link
Member Author

exitflynn commented Jan 28, 2023 •
edited
Loading

yes, i'm unsure of where I can get a file to test on since as of now we have an XRT header file and we do a get_dummy_map_from_header for testing. We could test based on this alone though...

@nabobalis
Copy link
Contributor

yes, i'm unsure of where I can get a file to test on since as of now we have an XRT header file and we do a get_dummy_map_from_header for testing. We could test based on this alone though...

Why would we not use that test header?

@exitflynn
Copy link
Member Author

oh no i was mistaken, i confused things with some stuff i did while testing locally 😅. yes we can do just that 👍

@exitflynn
Copy link
Member Author

is a changelog message like this one fine? 😬 👀

@exitflynn @nabobalis
minor fix in changelog
da1fee2 
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@exitflynn @nabobalis
improve changelog
7a8be70 
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@exitflynn
Copy link
Member Author

should be good to go now, i think

@nabobalis
Copy link
Contributor

It will need a review before but yes I think its ready to go.

wtbarnes
wtbarnes approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

Sorry, just a few last things that I should've caught previously.

sunpy/map/sources/hinode.py Outdated Show resolved Hide resolved
sunpy/map/sources/tests/test_xrt_source.py Outdated Show resolved Hide resolved
changelog/6725.bugfix.rst Outdated Show resolved Hide resolved
@wtbarnes wtbarnes self-requested a review January 31, 2023 20:14
wtbarnes
wtbarnes previously requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

Sorry, I meant to request changes, not approve.

exitflynn and others added 3 commits February 1, 2023 23:27
@exitflynn @wtbarnes
improve xrt unit detection
2753d02 
check for capitalization issues

Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn @wtbarnes
rename xrt unit test so that it runs
3b7f7d4 
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn @wtbarnes
improve changelog
3eacea2 
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@exitflynn
Copy link
Member Author

thanks for catching these!

@nabobalis nabobalis removed the request for review from wtbarnes February 1, 2023 22:17
@nabobalis
Copy link
Contributor

Since Will's comments are addressed, I will dismiss and merge this PR.

@nabobalis nabobalis merged commit 1702da0 into sunpy:main Feb 1, 2023
meeseeksmachine pushed a commit to meeseeksmachine/sunpy that referenced this pull request Feb 1, 2023
@exitflynn @meeseeksmachine
Backport PR sunpy#6725: add unit detection for XRT data
9e043d3
This was referenced Feb 1, 2023
Merged
Merged
meeseeksmachine pushed a commit to meeseeksmachine/sunpy that referenced this pull request Feb 1, 2023
@exitflynn @meeseeksmachine
Backport PR sunpy#6725: add unit detection for XRT data
c36b6fe
nabobalis pushed a commit that referenced this pull request Feb 1, 2023
@meeseeksmachine @exitflynn
Backport PR #6725 on branch 4.0 (add unit detection for XRT data) (#6760
5f98c40 
)

Co-authored-by: Akshit Tyagi <37214399+exitflynn@users.noreply.github.com>
@nabobalis
Copy link
Contributor

Thanks again @exitflynn

nabobalis pushed a commit that referenced this pull request Feb 1, 2023
@meeseeksmachine @exitflynn
Backport PR #6725 on branch 4.1 (add unit detection for XRT data) (#6761
ba94f89 
)

Co-authored-by: Akshit Tyagi <37214399+exitflynn@users.noreply.github.com>
@exitflynn
Copy link
Member Author

Thank you so much for the reviews @nabobalis and @wtbarnes! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabobalis nabobalis nabobalis approved these changes

@wtbarnes wtbarnes wtbarnes left review comments

Assignees
No one assigned
Labels
map Affects the map submodule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

XRT Maps do not include unit information
3 participants
@exitflynn @nabobalis @wtbarnes