nanograv / pint_pal

A long-lived repository for NANOGrav Pulsar Timing workflows and analysis.
MIT License
6 stars 19 forks source link

Fixed the error while setting tim drectory for excised tim file. #82

Closed lanky441 closed 3 weeks ago

lanky441 commented 1 month ago

As tim_directory is now a property of TimingConfiguration class with the setter function set_tim_directory, it was giving an error while setting the tim directory for excised tim file using self.tim_directory = ''. I have now changed it to self.set_tim_directory = '' to fix that.

rossjjennings commented 3 weeks ago

@lanky441 I introduced this bug in #63 by misunderstanding how setter functions work and failing to adequately test my code. I essentially wrote this:

class TimingConfiguration:
    @property
    def tim_directory(self):
        # get the tim directory
    @tim_directory.setter
    def set_tim_directory(self):
        # set the tim directory

thinking that the name of the set_tim_directory() function didn't matter, since the name of the Property was already set by the tim_directory() function. But in fact, the name of the setter does matter, and should have been tim_directory() for it to work in the usual way, where there is only one property tim_directory with both a getter and a setter, which is what I intended.

What the above code actually does is create two properties, one called tim_directory with only a getter function, and a second one called set_tim_directory with the exact same getter function, but an additional setter function. That's why getting the value with tc.tim_directory works fine, but to set it you have to do tc.set_tim_directory = ''. This is a byproduct of the way that property decorators work, where each one creates a new Property object that replaces the last one.

rossjjennings commented 3 weeks ago

The correct way to fix this is at the source, by changing the name of the setter to tim_directory() instead of set_tim_directory(). There's a couple of similar changes that I should make, too. I'll make a separate PR to do that, and close this one.