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

Chimera Functions #15

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

Chimera Functions #15

wants to merge 11 commits into from

Conversation

imogenagle
Copy link

No description provided.

CHIMERA_V2.py Outdated Show resolved Hide resolved
CHIMERA_V2.py Outdated Show resolved Hide resolved
CHIMERA_V2.py Outdated Show resolved Hide resolved
Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Hey this looks really good.

I think the main thing need to transfer these over to a new file in the package and also add doc strings and test

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.90%. Comparing base (910d3c4) to head (4c6404e).

❗ There is a different number of reports uploaded between BASE (910d3c4) and HEAD (4c6404e). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (910d3c4) HEAD (4c6404e)
3 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #15       +/-   ##
===========================================
- Coverage   97.61%   11.90%   -85.72%     
===========================================
  Files           1        1               
  Lines         252      252               
===========================================
- Hits          246       30      -216     
- Misses          6      222      +216     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Really good progress, some improvements to be made. Please don't get discouraged this is a process and getting much nearer the end now.

  • Map - should load the data into sunpy map and then use maps as much as possible
  • Function should take in arguments and reurn some thing e.g. thres_171 = threshold(171map, threshold) ideally not changing the inputs (if they do need to indicate this in the name/docstring)

I think it would be worth while to write out the major blocks/functions in an issue and what each function should take and return etc

rescale()
remove_neg()
threshold()  # maybe call clip
countour()  # better name - intensity ratio ?
create_mask() # better name - logarithmic_threshold ?

chimerapy/chimera.py Show resolved Hide resolved
pyproject.toml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This fixes the ruff/isort issue

chimerapy/chimera.py Show resolved Hide resolved
chimerapy/chimera.py Show resolved Hide resolved
chimerapy/chimera.py Show resolved Hide resolved
chimerapy/chimera.py Show resolved Hide resolved
chimerapy/chimera.py Show resolved Hide resolved
Comment on lines 10 to 59
from chimerapy.chimera import (
Bounds,
Xeb,
Xnb,
Xsb,
Xwb,
Yeb,
Ynb,
Ysb,
Ywb,
ang,
arcar,
arccent,
area,
cent,
centlat,
centlon,
chpts,
cont,
coords,
csys,
data,
datb,
datc,
datm,
dist,
eastl,
extent,
filter,
hg,
ins_prop,
mB,
mBneg,
mBpos,
npix,
pos,
remove_neg,
rescale_aia,
rescale_hmi,
set_contour,
sort,
threshold,
truarcar,
trummar,
trupixar,
westl,
width,
xpos,
ypos,
)
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
from chimerapy.chimera import (
Bounds,
Xeb,
Xnb,
Xsb,
Xwb,
Yeb,
Ynb,
Ysb,
Ywb,
ang,
arcar,
arccent,
area,
cent,
centlat,
centlon,
chpts,
cont,
coords,
csys,
data,
datb,
datc,
datm,
dist,
eastl,
extent,
filter,
hg,
ins_prop,
mB,
mBneg,
mBpos,
npix,
pos,
remove_neg,
rescale_aia,
rescale_hmi,
set_contour,
sort,
threshold,
truarcar,
trummar,
trupixar,
westl,
width,
xpos,
ypos,
)
from chimerapy.chimera import *

Comment on lines 8 to 13
INPUT_FILES = {
"aia171": "https://solarmonitor.org/data/2016/09/22/fits/saia/saia_00171_fd_20160922_103010.fts.gz",
"aia193": "https://solarmonitor.org/data/2016/09/22/fits/saia/saia_00193_fd_20160922_103041.fts.gz",
"aia211": "https://solarmonitor.org/data/2016/09/22/fits/saia/saia_00211_fd_20160922_103046.fts.gz",
"hmi_mag": "https://solarmonitor.org/data/2016/09/22/fits/shmi/shmi_maglc_fd_20160922_094640.fts.gz",
}
Copy link
Member

Choose a reason for hiding this comment

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

you need to use this as the files won't be in the local directory

Comment on lines +284 to +282
for i in range(len(cont)):
x = np.append(x, len(cont[i]))
Copy link
Member

Choose a reason for hiding this comment

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

ideally dont want to test in loop like this

Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Lots of good stuff in here still some improvements to be made

  • Map - should load the data into sunpy map and then use maps as much as possible
  • Function should take in arguments and reurn some thing e.g. thres_171 = threshold(171map, threshold) ideally not changing the inputs
  • Write out high level functions and there input and outputs (maybe in an new issue)
rescale()
remove_neg()
threshold()  # maybe call clip
countour()  # maybe call intensity_ratio
create_mask() #  maybe call threshold

Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Nice some good improvements left some more detailed comments.

Few general things map isn't a great name as there is python builtin function called map so amap or inmap or similar is usually better.

Want to make use of the units so avoid dropping if you can and when you do have to get a number use to_value(u.arcsec) because maybe the value as set in kiloarcsec or milliarcsec and better to not assume.

Comment on lines 18 to 24
# --noverify

plt.style.use(astropy_mpl_style)

"""Defining the paths for example files used to run program locally"""

file_path = "./"
Copy link
Member

Choose a reason for hiding this comment

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

Delete?

Comment on lines 55 to 60
new_x_scale = map1.scale[0].to(u.arcsec / u.pixel).value
new_y_scale = map1.scale[1].to(u.arcsec / u.pixel).value
map1.meta["cdelt1"] = new_x_scale
map1.meta["cdelt2"] = new_y_scale
map1.meta["cunit1"] = "arcsec"
map1.meta["cunit2"] = "arcsec"
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't have to do this?

Copy link
Author

Choose a reason for hiding this comment

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

when you do reproject, it changes the plate scale and this was the only way I could figure out how to change the plate scale back

Copy link
Member

Choose a reason for hiding this comment

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

I think this is related to the above comment, as after reprojecting to the same WCS the maps should have the WCS information including the pixel size? Before you were reprojecting the 171 image to 193, 211 wcs rather than the other way around.

return map1


im171 = rescale(im171, im171)
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to reproject the 171 map to itself


"""
with propagate_with_solar_surface():
map1 = proj_to.reproject_to(input_map.wcs)
Copy link
Member

Choose a reason for hiding this comment

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

So this is going the opposite way around I think you want but see above comment too.

Suggested change
map1 = proj_to.reproject_to(input_map.wcs)
map1 = input_map.reproject_to(proj_to.wcs)

imhmi = Map(INPUT_FILES["hmi_mag"])


def rescale(proj_to: sunpy.map.Map, input_map: sunpy.map.Map):
Copy link
Member

Choose a reason for hiding this comment

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

To make this more explicit

Suggested change
def rescale(proj_to: sunpy.map.Map, input_map: sunpy.map.Map):
def reproject_diff_rot(target_wcs: astropy.wcs.wcs.WCS, input_map: sunpy.map.Map):

CHIMERA_V3.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Delete too

Comment on lines 96 to 100
"""defines the shape of the arrays as "s" and "rs" as the solar radius"""
s = np.shape(im171.data)
# do we want the solar radius in arcsec or pixels?
rs = im171.rsun_obs
rs_pixels = im171.rsun_obs / im171.scale[0]
Copy link
Member

Choose a reason for hiding this comment

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

anything like this needs to be in a function e.g.

def chimera(maps, ...):
   ...
   im171, im193, im211 = filter(im171, im193, im211)
   
   # defines the shape of the arrays as "s" and "rs" as the solar radius
   s = np.shape(im171.data)
   # do we want the solar radius in arcsec or pixels?
   rs = im171.rsun_obs
   rs_pixels = im171.rsun_obs / im171.scale[0]

   dattoarc, conver, convermul = pix_arc(im171)


"""
dattoarc = map.scale[0].value
conver = ((s[0]) / 2) * dattoarc / map.meta["cdelt1"] - (s[1] / 2)
Copy link
Member

Choose a reason for hiding this comment

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

map.meta["cdelt1"] is map.scale[0] so this dattoarc / map.meta["cdelt1"] should be 1 then this entire line should be 0, but the units don't make sense either

m171nrt = Map('http://jsoc.stanford.edu/data/aia/synoptic/2016/09/22/H1000/AIA20160922_1030_0171.fits')
s = m171nrt.dimensions
(s.x/2) * m171nrt.scale[0] /  m171nrt.meta["cdelt1"], (s.y / 2)
(<Quantity 512. arcsec>, <Quantity 512. pix>)

Comment on lines 163 to 184
pix_size = (2000 * u.arcsec).value
garr = Gaussian2D(
1,
im171.reference_pixel.x.value,
im171.reference_pixel.y.value,
pix_size / im171.scale[0].value,
pix_size / im171.scale[0].value,
)(x, y)
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
pix_size = (2000 * u.arcsec).value
garr = Gaussian2D(
1,
im171.reference_pixel.x.value,
im171.reference_pixel.y.value,
pix_size / im171.scale[0].value,
pix_size / im171.scale[0].value,
)(x, y)
width = (2000 * u.arcsec)
garr = Gaussian2D(
1,
im171.reference_pixel.x.to_value(u.pix),
im171.reference_pixel.y.to_value(u.pix),
width / im171.scale[0],
width / im171.scale[1],
)(x, y)

Comment on lines 499 to 512
centlat = hg.lat[int(ypos), int(xpos)]
centlon = hg.lon[int(ypos), int(xpos)]
Copy link
Member

Choose a reason for hiding this comment

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

Where do ypos, xpos come from there they should be function arguments if they are used?

Copy link
Author

Choose a reason for hiding this comment

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

They were inputs in my last version - not sure why I deleted them but I'm including them again now

Copy link
Author

Choose a reason for hiding this comment

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

Just looked a little more closely, they are the x and y positions of the center point which are used later in the for loop. I assumed we'd change that so they weren't calculated manually which I think is why I got rid of them.

… document attached at the end in a link that describes all of the important processes, functions, and variables
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