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

Ultra l1b extended events #686

Open
wants to merge 36 commits into
base: dev
Choose a base branch
from

Conversation

laspsandoval
Copy link
Contributor

@laspsandoval laspsandoval commented Jul 3, 2024

Change Summary

Overview

All of l1b de except any SPICE utilization. Note that I still don't have recent lookup tables so I will update this code once I have them.

New Files

lookup_utils.py

  • Contains tools for lookup tables for l1b.

ultra_l1b_extended.py

  • Calculates extended raw events for Ultra l1b.

/lookup_tables (5 files)

imap_processing/tests/ultra/test_data/l0/FM45_40P_Phi28p5_BeamCal_LinearScan_phi28.50_theta-0.00_Ultra_Image_Raw_Event_20240207T102746_withFSWcalcs.csv

  • test data

Updated Files

imap_ultra_l1b_variable_attrs.yaml

  • Added and improved variable descriptions.

conftest.py

  • Added some test data.

de.py

  • Added direct event data.

Testing

test_lookup_utils.py

  • Tests lookup_utils.py

test_ultra_l1b_extended.py

  • Tests ultra_l1b_extended.py

test_ultra_l1b.py

  • Tests ultra_l1b.py

Environment

poetry.lock
pyproject.toml

@laspsandoval laspsandoval self-assigned this Jul 3, 2024
@laspsandoval laspsandoval requested a review from a team July 3, 2024 22:07
Copy link
Contributor

@subagonsouth subagonsouth left a comment

Choose a reason for hiding this comment

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

Great to see some algorithm code being implemented.

This is a very large PR. I was unable to to a thorough review but provided what feedback I could.

imap_processing/ultra/l1b/de.py Outdated Show resolved Hide resolved
imap_processing/ultra/l1b/de.py Outdated Show resolved Hide resolved
imap_processing/ultra/l1b/lookup_utils.py Outdated Show resolved Hide resolved
imap_processing/ultra/l1b/lookup_utils.py Outdated Show resolved Hide resolved
imap_processing/ultra/l1b/lookup_utils.py Show resolved Hide resolved
@laspsandoval laspsandoval requested a review from sdhoyt July 19, 2024 15:00
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

This is too much for me to review in one sitting, but I left some general comments now.

I'd really like to see this broken up into roughly each individual function as a PR, which means this one PR turns into ~15-20. I think this would go much faster and easier for getting feedback on as well.

# I suspect that the lookup table is different.
test_xc = df_filt["Xc"].iloc[index].astype("float")
etof, xc = get_coincidence_positions(de_dataset, tof, "ultra45")
assert xc == pytest.approx(test_xc.values, rel=1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and elsewhere in the tests it would be helpful to use the numpy testing utilities for array comparisons.
https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_allclose.html

np.testing.assert_allclose(xc, test_xc.values, rtol=...)

Comment on lines +180 to +181
# TODO: the first value doesn't match. Look into this.
assert np.array_equal(test_energy[1::], energy[1::].astype(float))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it should have an xfail/skip associated with it. A TODO in the tests doesn't seem ideal as I'm not sure we'll get back to it, so it could get lost in time.

Suggested change
# TODO: the first value doesn't match. Look into this.
assert np.array_equal(test_energy[1::], energy[1::].astype(float))
# TODO: the first value doesn't match. Look into this.
np.testing.assert_array_equal(test_energy[1::], energy[1::].astype(float))


# Pulse height
ph_indices = np.where(
(de_dataset["STOP_TYPE"] == 1) | (de_dataset["STOP_TYPE"] == 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should STOP_TYPE be an Enum? What do 1 and 2 mean here?

# Pulse height
ph_indices = np.where(
(de_dataset["STOP_TYPE"] == 1) | (de_dataset["STOP_TYPE"] == 2)
)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You only want the first occurrence, or do you want all sets of indices matching this condition?
Numpy also suggests using nonzero() instead to get the indices. https://numpy.org/doc/stable/reference/generated/numpy.where.html

ph_indices = np.nonzero(de_dataset["STOP_TYPE"] == 1) | (de_dataset["STOP_TYPE"] == 2)

ph_etof, ph_xc = get_coincidence_positions(de_dataset, ph_tof, "ultra45")

# SSD
ssd_indices = np.where(de_dataset["STOP_TYPE"] >= 8)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest changing all "where" to nonzero and making sure you only want the first occurrence or if you want all of the indices. (indices to me means there could be multiples whereas you're only grabbing the first ssd_index currently)

Suggested change
ssd_indices = np.where(de_dataset["STOP_TYPE"] >= 8)[0]
ssd_indices = np.nonzero(de_dataset["STOP_TYPE"] >= 8)

@@ -5,7 +5,7 @@ build-backend = "poetry_dynamic_versioning.backend"
[tool.poetry]
name = "imap-processing"
# Gets updated dynamically by the poetry-dynamic-versioning plugin
version = "0.0.0"
version = "0.3.0.post24.dev0+517786f"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be left as "0.0.0" and not updated, see the comment above. There is something going wrong with your install if this is getting committed I think.

t2 = np.zeros(len(de_dataset["STOP_TYPE"]))
tof = np.zeros(len(de_dataset["STOP_TYPE"]))

# Stop Type: 1=Top, 2=Bottom
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest an Enum for this. STOP_TYPE.Top, STOP_TYPE.Bottom or similar

# For the stop anode, there are mismatches between the coincidence TDCs,
# i.e., CoinN and CoinS. They must be normalized via lookup tables.

# TpCoinNNorm Top
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a lot of duplicated code for the two cases here, so would it make sense to try and factor some of this out? I think you could also subset your dataset instead of keeping track of the indices and using those all the itme.
https://docs.xarray.dev/en/v2024.06.0/generated/xarray.Dataset.where.html

top_ds = de_dataset.where(de_dataset["COIN_TYPE"] == 1)

@laspsandoval
Copy link
Contributor Author

laspsandoval commented Jul 22, 2024

This is too much for me to review in one sitting, but I left some general comments now.

I'd really like to see this broken up into roughly each individual function as a PR, which means this one PR turns into ~15-20. I think this would go much faster and easier for getting feedback on as well.

Ok. I'll create separate branches to break it up.

@greglucas greglucas added Ins: Ultra Related to the IMAP-Ultra instrument Level: L1 Level 1 processing labels Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ins: Ultra Related to the IMAP-Ultra instrument Level: L1 Level 1 processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants