Using instance method in data class

[seongsujeong]

Here is some context to understand the situation:
In the last PR #48, I've added few data classes into Sentinel1BurstSlc as its attributes (see below):

https://github.com/opera-adt/s1-reader/blob/3c5ab381e647d9a93dc0924a49f460c411ddd3ef/src/s1reader/s1_burst_slc.py#L169-L172

They contain the burst-wise information, whose information are extracted subswath-wide annotation dataclasses (i.e. CalibrationAnnotation, NoiseAnnotation, etc.) Below is the example for BurstCalibration.from_calibration_annotation().:

https://github.com/opera-adt/s1-reader/blob/3c5ab381e647d9a93dc0924a49f460c411ddd3ef/src/s1reader/s1_annotation.py#L443-L444

As it can be shown above, the extractor has a decorator @classmethod

Running s1reader.load_bursts() will give the list of Sentinel1BurstSlc instances. The information inside the attributes above has to be unique for each instances in the list, because each bursts have different parameters for noise correction, radiometric calibration, etc. However, making the extractor function as a classmethod make the values identical because all instances shares the attributes as class attributes.

Such issue does not happen when the attributes are initialized when the instances are generated (see below). Same approaches can be borrowed for the burst-wise dataclass, but it will make the function s1_reader.burst_from_xml() too long.
https://github.com/opera-adt/s1-reader/blob/3c5ab381e647d9a93dc0924a49f460c411ddd3ef/src/s1reader/s1_reader.py#L498-L510

One of the workaround I did in the PR #48 was to define the extractor as an instance method. Below is the example of BurstNoise.from_noise_annotation() as below:
https://github.com/opera-adt/s1-reader/blob/3c5ab381e647d9a93dc0924a49f460c411ddd3ef/src/s1reader/s1_annotation.py#L367-L368

In that context, my questions are:

• I know it is allowed to use instance method for dataclass, but is it a good practice?
• Would my approach above (i.e. using instance method) the best way to implement the burst-wise correction/calibration info, while keeping the method s1_reader.burst_from_xml() simple?

[LiangJYu]

I think the approach used to init and assign az_fm_rate_list and doppler_list can be applied to simplify calibration and noise read and load. Just like calibration and noise, both of the aforementioned attributes exist as lists in XML. Instead of having to two dataclasses (XAnnotation and X), the elements in each element list are parsed and saved to their respective objects without any intermediate object. In addition, XAnnotation and X are essentially the same, thus making one redundant.

The following is how az_fm_rate_list is read and loaded:

1. Fix relevant element and iterate over it's elements to create an object for each element.
   https://github.com/opera-adt/s1-reader/blob/3c5ab381e647d9a93dc0924a49f460c411ddd3ef/src/s1reader/s1_reader.py#L398-L400
2. While constructing each burst, find nearest az_fm_rate.
   https://github.com/opera-adt/s1-reader/blob/3c5ab381e647d9a93dc0924a49f460c411ddd3ef/src/s1reader/s1_reader.py#L460

The same process above is loosely applied to noise below. The treatment for calibration would be very similar.

1. Replace from_noise_annotation in dataclass BurstNoise with @classmethod from_elem. It'll be something like:

@classmethod
def from_elem(cls, elem):
    cls.something = float(elem.find('something')) # float in the case is the expected type
    ...
    # do ipf specific things
    return (time_stamp, cls) 

2. Iterate over Noise element tree and create a list of Noise objects

with open_method(noise_annotation_path, 'r') as f_nads:
    ET.parse(f_nads).find('noiseVectorList')
    noise_list = [BurstNoise.from_elem(e) for e in et]

3. Find (using something like get_nearest_polynomial) and use nearest BurstNoise object when building burst.

Let me know what you think.

[seongsujeong]

@LiangJYu Thank you for the opinion about how the code can be improved. I agree that there are some redundancy in the current version of s1_annotation.py, for which the code needs to be improved down the road.

My concern in using @classmethod is that it is a class method. It seems the instances generated by class method share the same attribute (i.e. class attribute).
The code below is the example:

from dataclasses import dataclass

@dataclass
class DummyDataClass:
    attr_a: int = None
    
    @classmethod
    def from_int(cls, val_in):
        cls.attr_a = val_in

        return cls

if __name__ == '__main__':
    ITERABLE_OBJ = range(5)
    
    list_dummy_instance = [DummyDataClass.from_int(i) for i in ITERABLE_OBJ]

    print([dummy_instance.attr_a for dummy_instance in list_dummy_instance])


# output: [4, 4, 4, 4, 4]

The simplest solution I could think about was to make it instance method, so that each instances from BurstNoise has its own information.

[LiangJYu]

There's bug in your example. The classmethod should be:

    @classmethod
    def from_int(cls, val_in):

        return cls(val_in)

I think the correction above returns a new instance of cls by calling its constructor. Whereas your example returns the same object repeatedly and the print values are identical.

For BurstAnnotation, instead of repeatedly assigning cls.x = some_parsed_value and then return cls, the classmethod should define all the parsed values and then use them in the cls constructor and return object created by the constructor.

@classmthod
def from_et(cls, et):
    something = parse_something(et)
    something_else = parse_something_else(et)
    another_thing = parse_another_thing(et)
    ....
    return cls(something, something_else, another_thing)

[LiangJYu]

Sorry I went way off topic with my 1st response. I really appreciate the simple example in your follow up. It made perfectly clear the matter at hand.

[seongsujeong]

@LiangJYu Thanks For the suggestion. I have not thought about returning the cls in that way. It looks your correction above works as intended.
Aside from the redundancy removal (in your 1st reply), I think the the modification on the current codes would not be extensive. I appreciate the solution!