Skip to content

Conversation

@levi178u
Copy link
Contributor

@levi178u levi178u commented Apr 2, 2025

Purpose of PR?:

Fixes #2750

fix: add missing super().init() calls

This PR adds the missing super().__init__() calls in MFDatasetCommonDims and
WebMapService to ensure proper parent class initialization.
Ensured proper initialization of the parent class netCDF4.MFDataset, improving compatibility
and following rules of OOPs (object-oriented principles).

@ReimarBauer ReimarBauer requested a review from matrss April 2, 2025 19:30
@levi178u
Copy link
Contributor Author

levi178u commented Apr 2, 2025

@ReimarBauer and @matrss , Here basically I have done changes such as initialized object states with certain conditions (like In case of unlimited dimensions of NetCDF file, ) superclass or made added prefix '_' to make sure it follows the calling principle of inheritance. pls review the PR and what changes shud I make more as it failed on create Gallery tc ?

Copy link
Collaborator

@matrss matrss left a comment

Choose a reason for hiding this comment

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

Both classes seem to have at least parts of their constructors copied over from the classes they inherit, so just calling the super classes constructor is not enough, it has to be checked what that constructor does and what ours do so far, what the differences are, and how to achieve the same result with a cleaner refactoring of these classes.


def __init__(self, files, exclude=None, skip_dim_check=None,
require_dim_num=False):
super().__init__(files)
Copy link
Collaborator

Choose a reason for hiding this comment

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

All method code belongs below its docstring.

Comment on lines 252 to 258
if_unlimited_dim = any(dim.isunlimited() for dim in cdfm.dimensions.values())
if not if_unlimited_dim:
super().__init__()
self._cdf = [cdfm]
self._isopen = True
else:
super().__init__(files)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Together with the super().__init__ call at the top this will always result in calling the super classes constructor twice. That can't be right.

@levi178u
Copy link
Contributor Author

levi178u commented Apr 3, 2025

Both classes seem to have at least parts of their constructors copied over from the classes they inherit, so just calling the super classes constructor is not enough, it has to be checked what that constructor does and what ours do so far, what the differences are, and how to achieve the same result with a cleaner refactoring of these classes.

@matrss thanks for the detailed review I'll make the mentioned changes and additions with a fresh commit in the PR very soon and also refractor the parent class constructor with updated calling

@levi178u
Copy link
Contributor Author

levi178u commented Apr 12, 2025

@matrss pls review this PR and the changes ,

  • I have written the super().__int__() part below the docstrings
  • double super().__init__() call in MFDatasetCommonDims constructor
  • Use conditional initialization based on presence of unlimited dimensions.

Also, what should I do the fix other issues occurring here

@levi178u levi178u requested a review from matrss April 12, 2025 06:00
@levi178u
Copy link
Contributor Author

@matrss and @ReimarBauer pls review this PR , i have done some changes

@levi178u
Copy link
Contributor Author

@matrss can i know why is this error occurring ? i think it's not related to the issue
image

@matrss
Copy link
Collaborator

matrss commented Apr 24, 2025

Well, it doesn't happen without your changeset, so it must be related somehow.

@levi178u
Copy link
Contributor Author

Well, it doesn't happen without your changeset, so it must be related somehow.

May I know what changes must be done to fix the same , as I'm unable to get this error

@matrss
Copy link
Collaborator

matrss commented Apr 29, 2025

I was able to reproduce the error by following what the failing workflow does: https://github.com/Open-MSS/MSS/actions/runs/14640654490/workflow?pr=2781.

On the stable branch this error doesn't happen.

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.

2 participants