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

BUG: fix loading of AMReX datasets with ~ in the filename #4943

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

yut23
Copy link
Member

@yut23 yut23 commented Jul 8, 2024

PR Summary

One of our group members ran into this today when doing ds = CastroDataset('~/delta_ims/en_20/planar_plt02346'), with an error like:

yt/frontends/boxlib/data_structures.py", line 1116, in _parse_parameter_file
    jobinfo_filename = os.path.join(self.output_dir, self.cparam_filename)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen posixpath>", line 90, in join
  File "<frozen genericpath>", line 164, in _check_arg_types
TypeError: join() argument must be str, bytes, or os.PathLike object, not 'NoneType'

self._lookup_cparam_filepath() was being passed the version of output_dir before having the tilde expanded, and was returning None as ./~/ doesn't exist.

I'm not sure how to add a test for this.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.

@yut23 yut23 added code frontends Things related to specific frontends bug labels Jul 8, 2024
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

The patch is simple enough that I don't think we need a test for it. But I think this class should reuse code from its parent instead of reimplementing identical yet buggy logic.

@@ -652,7 +652,7 @@ def __init__(

cparam_filename = cparam_filename or self.__class__._default_cparam_filename
self.cparam_filename = self._lookup_cparam_filepath(
output_dir, cparam_filename=cparam_filename
self.output_dir, cparam_filename=cparam_filename
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to use self.directory instead provided the parent class' __init__ is called first. Can you try that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately that doesn't work: Dataset.__init__() calls self._parse_parameter_file(), which needs self.cparam_filename to be set beforehand.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough !

@neutrinoceros neutrinoceros added this to the 4.4.0 milestone Jul 9, 2024
@neutrinoceros neutrinoceros merged commit c0c0bbb into yt-project:main Jul 9, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code frontends Things related to specific frontends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants