Better management of Trace attribute mutability

[bmorris3]

#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]

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]

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]

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]

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

[bmorris3]

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)

[ojustino]

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.

[ojustino]

Is this just for alphabetization purposes?

[ojustino]

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

[hpparvi]

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.