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

Better management of Trace attribute mutability #147

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

Better management of Trace attribute mutability #147

wants to merge 4 commits into from

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Oct 25, 2022 •
edited
Loading

#126 points out that you can edit attributes of the Trace class in such a way that they are no longer self-consistent with each other.

In this PR, I've found a way to recast the *Traces as subclasses of a new BaseTrace (perhaps there's a better name?). These classes all use @dataclass(frozen=True) which enforces that their attributes are immutable for the user-facing API. Under the hood, there's a slightly ugly workaround which allows us to (re)set these "read-only" attributes via the public-facing methods like Trace.shift, for example.

Happy to discuss as needed!

Fixes #126

Tiny demo

import pytest
from specreduce.utils.synth_data import make_2dspec_image
from specreduce.tracing import FlatTrace

img = make_2dspec_image()

trace = FlatTrace(img, 10)
print(trace.trace_pos, trace.trace[0])
# used to produce: 10, 10
# now produces: 10, 10

trace_shifted = trace + 10
print(trace_shifted.trace_pos, trace_shifted.trace[0])
# used to produce: 10, 20
# now produces: 20, 20

with pytest.raises(AttributeError):
    # this didn't raise an error before, but does now
    trace_shifted.trace_pos = 15

@codecov
Copy link

codecov bot commented Oct 25, 2022 •
edited
Loading

Codecov Report

Merging #147 (46c33a6) into main (86d501c) will increase coverage by 0.97%.
The diff coverage is 95.08%.

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   71.63%   72.60%   +0.97%     
==========================================
  Files           9        9              
  Lines         631      657      +26     
==========================================
+ Hits          452      477      +25     
- Misses        179      180       +1
Impacted Files Coverage Δ
specreduce/tracing.py 91.41% <94.91%> (+0.90%) ⬆️
specreduce/background.py 89.28% <100.00%> (ø)
specreduce/extract.py 88.32% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tepickering
Copy link
Contributor

looks pretty good! i need to take a deeper dive, but one substantive comment is that trace_pos is only really meaningful for FlatTrace and shouldn't be in the base class. in fact, it should just be an InitVar for FlatTrace that is used to construct self._trace and not become an attribute. if we keep it around as a property, it should get the position directly from self._trace.

@kecnry
Copy link
Member

kecnry commented Oct 26, 2022

This is a clever workaround, and in general I like it. But before moving forward in this direction, I think we need to open a wider discussion over whether we want to support the user setting any attributes across any of the classes (background and extraction included) and expect the internal information in those objects to update respectively. Currently I think the user can set any of those attributes, but similar to the cases here, internal arrays aren't updated accordingly, so I think we can decide to go either direction as long as we do so consistently across the board. But if we do want some/most to be editable, then would we need to set custom setters for each case or would that make this approach unfeasible?

As a side note, correct me if I'm wrong @tepickering, but I think Trace already was the base class and wasn't meant to be directly by the user... so maybe we wouldn't need separate BaseTrace and Trace classes here.

@pllim
Copy link
Member

pllim commented Nov 3, 2022

I don't see any test. Shouldn't there be tests to ensure this patch fixes the problem?

@bmorris3
Copy link
Contributor Author

bmorris3 commented Nov 3, 2022

In the last two commits I've:

  • removed BaseTrace and implemented the same functionality on the Trace class, thanks @kecnry (comment)
  • moved the trace_pos attribute to be specific to only the FlatTrace subclass, thanks @tepickering (comment)
  • added tests, thanks @pllim (comment)

@kecnry kecnry mentioned this pull request Nov 8, 2022
Open
ojustino
ojustino reviewed Nov 8, 2022
View reviewed changes
Copy link
Contributor

@ojustino ojustino left a comment

Choose a reason for hiding this comment

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

Thanks for your work, @bmorris3. I found this approach a little hard to follow at first. My understanding now is it seems it has to be a backdoor-style fix due to some limitations of dataclass, so I can get behind it.

I think it makes sense for most (if not all) of our current selection of attributes to be read-only, so I support an approach like the one here over the alternative of allowing editable attributes and needing to update internal arrays. I feel the same about the non-trace operations, too, but that work would be for another ticket.

I agree that we wouldn't need both a BaseTrace and Trace, so it's good we're sticking with Trace as the base class.

specreduce/background.py
@@ -8,7 +8,7 @@
from astropy import units as u

from specreduce.extract import _ap_weight_image, _to_spectrum1d_pixels
from specreduce.tracing import Trace, FlatTrace
from specreduce.tracing import FlatTrace, Trace
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just for alphabetization purposes?

specreduce/tracing.py
@@ -92,10 +109,12 @@ class FlatTrace(Trace):
trace_pos : float
Position of the trace
"""
trace_pos: float
_trace_pos: (float, np.ndarray) = field(repr=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand when/why this FlatTrace argument would take an array?

@ojustino ojustino mentioned this pull request Nov 15, 2022
Merged
@hpparvi
Copy link
Contributor

hpparvi commented Nov 27, 2024

Hi @bmorris3. Do you remember if there was any off-GitHub discussion that led to this PR not being merged? I like the idea of making the tracing classes immutable and would be happy to help get this merged, though probably only after @cshanahan1's masking work.

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

@ojustino ojustino ojustino left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FlatTrace: possible points of confusion when editing existing object
6 participants
@bmorris3 @tepickering @kecnry @pllim @hpparvi @ojustino