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

Reimplement dataaccessor #66

Merged
merged 10 commits into from
Aug 16, 2024
Merged

Reimplement dataaccessor #66

merged 10 commits into from
Aug 16, 2024

Conversation

HannoSpreeuw
Copy link
Collaborator

Following @suvayu on replacing DataAccessor by a dataclass.
DataAccessor was inheriting from RequiredAttributesMetaclass.
This way of enforcing required attributes seems too heavy machinery and the dataclass is a a lightweight solution.

Should also fix #53

… metaclass=RequiredAttributesMetaclass), i.e. DataAccessor inheriting from RequiredAttributesMetaclass is an overkill, since RequiredAttributesMetaclass does nothing more than checking for required attributes. requiredatts.py can be removed and DataAccessor can be turned into a dataclass, which will have the required arguments declared.
…ired attributes is much simpler achieved by defining a Python dataclass instead of inheriting from a metaclass.
… parent class of FitsImage is a dataclass. In this case that will yield: TypeError: DataAccessor.__init__() missing 11 required positional arguments:....
@HannoSpreeuw HannoSpreeuw self-assigned this Aug 15, 2024
@HannoSpreeuw
Copy link
Collaborator Author

Probably some loose ends although all unit tests pass, since hatch run lint:mypy yields

sourcefinder/utility/containers.py:55: error: Signatures of "__iadd__" and "__add__" are incompatible  [misc]
sourcefinder/accessors/lofarhdf5image.py:9: error: Missing positional arguments "beam", "centre_ra", "centre_decl", "data", "freq_bw", "freq_eff", "pixelsize", "tau_time", "taustart_ts", "url", "wcs" in call to "__init__" of "DataAccessor"  [call-arg]
sourcefinder/accessors/lofarhdf5image.py:62: error: "LofarHdf5Image" has no attribute "header"  [attr-defined]
sourcefinder/accessors/fitsimageblob.py:45: error: Signature of "_get_header" incompatible with supertype "FitsImage"  [override]
sourcefinder/accessors/fitsimageblob.py:45: note:      Superclass:
sourcefinder/accessors/fitsimageblob.py:45: note:          def _get_header(self, hdu_index: Any) -> Any
sourcefinder/accessors/fitsimageblob.py:45: note:      Subclass:
sourcefinder/accessors/fitsimageblob.py:45: note:          def _get_header(self, hdulist: Any, hdu_index: Any) -> Any
sourcefinder/accessors/fitsimageblob.py:48: error: Signature of "read_data" incompatible with supertype "FitsImage"  [override]
sourcefinder/accessors/fitsimageblob.py:48: note:      Superclass:
sourcefinder/accessors/fitsimageblob.py:48: note:          def read_data(self, hdu_index: Any, plane: Any) -> Any
sourcefinder/accessors/fitsimageblob.py:48: note:      Subclass:
sourcefinder/accessors/fitsimageblob.py:48: note:          def read_data(self, hdulist: Any, hdu_index: Any, plane: Any) -> Any
Found 5 errors in 3 files (checked 30 source files)

@HannoSpreeuw
Copy link
Collaborator Author

The line
sourcefinder/accessors/lofarhdf5image.py:61: error: "LofarHdf5Image" has no attribute "header" [attr-defined] from the mypy output has to do with

    def get_header(self):
        # Preserved for API compatibility.
        return self.header

Apparently not implemented yet.

Should not be solved as part of this PR.

@HannoSpreeuw
Copy link
Collaborator Author

The other mypy complaints have been taken care of through commit d163f9b.

Copy link
Collaborator

@suvayu suvayu left a comment

Choose a reason for hiding this comment

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

Hi @HannoSpreeuw, I'll approve these for now since the tests pass. But I don't think this is completely there yet. There are some inconsistencies that I would like to address. I'll open a separate issue to collect everything I spotted, and we can address that in a separate PR.

Because of the above reason and #65, maybe #53 should still be open.

@HannoSpreeuw
Copy link
Collaborator Author

Allright.
So that means DataAccessor has been adequately reimplemented, but the design of its derived classes may need improvements.

@HannoSpreeuw HannoSpreeuw merged commit 0566ae8 into master Aug 16, 2024
4 of 6 checks passed
@HannoSpreeuw HannoSpreeuw deleted the reimplement_dataaccessor branch August 16, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reimplement DataAccessor for easier testing & validation
2 participants