Path.replace bug

[Sumit112192]

📝 In file tardis/io/atom_data/util.py

fname sometimes becomes a path object causing the bug, and on line 38, we seem to apply the replace method of str on the path object, which has its own replace method.

In the file tardis/simulation/base.py on line 665, atom_data_fname becomes a path object, causing this error. A better way would be to make atom_data_fname str here. Could anyone suggest a better wat to fix this bug?

tardis/tardis/simulation/base.py

Lines 660 to 667 in 39c128a

	if atom_data is None:
	if "atom_data" in config:
	if Path(config.atom_data).is_absolute():
	atom_data_fname = config.atom_data
	else:
	atom_data_fname = (
	Path(config.config_dirname) / config.atom_data
	)

Type: 🪲 bugfix

📌 Resources

Examples, notebooks, and links to useful references.

[tardis-bot]

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

[Sumit112192]

@wkerzendorf @atharva-2001 Can you please review this request? Thanks.

[sarthak-dv]

That's a great catch @Sumit112192 !

I think another way is that instead of manually replacing ".h5" with "" (which seems a slightly dirty thing to do), we stick to Path object, as use stem function, as below:

In [32]: path
Out[32]: PosixPath('/Users/sarthak/Downloads/DL_back.jpeg')

In [33]: path.parent.joinpath(path.stem)
Out[33]: PosixPath('/Users/sarthak/Downloads/DL_back')

Of course we'll have to make sure that this change is backward compatible, i.e. the function resolve_atom_data_fname continues to accept path and strings too.

[andrewfullard]

Could you instead ensure that fname is always a Path object instead of casting to a string?

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 68.06%. Comparing base (3907189) to head (29516d2).
Report is 1 commits behind head on master.

Files	Patch %	Lines
tardis/io/atom_data/util.py	40.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
- Coverage   68.06%   68.06%   -0.01%     
==========================================
  Files         166      166              
  Lines       14161    14164       +3     
==========================================
+ Hits         9639     9641       +2     
- Misses       4522     4523       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Sumit112192]

In the below code snippet,
inside the body of the if statement, fname is initialized as a Str object ( Line 663 ), and
inside the body of the else statement, fname is initialized as a Path object ( Line 666 ).

tardis/tardis/simulation/base.py

Lines 661 to 667 in f515d16

	if "atom_data" in config:
	if Path(config.atom_data).is_absolute():
	atom_data_fname = config.atom_data
	else:
	atom_data_fname = (
	Path(config.config_dirname) / config.atom_data
	)

In the resolve_atom_data_frame function

tardis/tardis/io/atom_data/util.py

Lines 13 to 36 in f515d16

	def resolve_atom_data_fname(fname):
	"""
	Check where if atom data HDF file is available on disk, can be downloaded or does not exist
	Parameters
	----------
	fname : str
	name or path of atom data HDF file
	Returns
	-------
	: str
	resolved fpath
	"""
	if os.path.exists(fname):
	return fname
	fpath = os.path.join(os.path.join(get_data_dir(), fname))
	if os.path.exists(fpath):
	logger.info(
	f"\n\tAtom Data {fname} not found in local path.\n\tExists in TARDIS Data repo {fpath}"
	)
	return fpath

It's supposed to have str as input and str as output.

Should I instead convert the Path object on line 666 to a Str object as that seems to resolve the issue and carry on the before objective of resolve_atom_data_fname

@andrewfullard @sarthak-dv

[andrewfullard]

I would prefer if you modify resolve_atom_data_fname to use Path objects instead

[Sumit112192]

@andrewfullard Okay, I will do that.

[andrewfullard]

@Sumit112192 do you have an expected timeline for this PR?

[Sumit112192]

I am really sorry for the delay. I had some college work to do this week. I will start working on it from Friday night. @andrewfullard

[andrewfullard]

Please run black on your changes.

[Sumit112192]

@andrewfullard Done as suggested by black.

[andrewfullard]

Thanks for this fix!

[DeerWhale]

Thanks for spotting the bug and the fix! I think this fix the case when the atom data file in the same directory as the config file, however I still get error when I only specific the name of the atom_data file (not the absolute path) that exist in the tardis_data folder, which should still work as one of the check in the linked part of code below:

tardis/tardis/io/atom_data/util.py

Lines 31 to 36 in ab869ea

	fpath = os.path.join(os.path.join(get_data_dir(), fname))
	if os.path.exists(fpath):
	logger.info(
	f"\n\tAtom Data {fname} not found in local path.\n\tExists in TARDIS Data repo {fpath}"
	)
	return fpath

Could you attempt to fix those cases as well? Thanks!

[DeerWhale]

Since fname is an absolute path coming from "atom_data_fname" in simulation/base.py instead of the file name only, should we use fname.stem here as suggested by #2535 (comment)?

[Sumit112192]

Thanks for spotting the bug and the fix! I think this fix the case when the atom data file in the same directory as the config file, however I still get error when I only specific the name of the atom_data file (not the absolute path) that exist in the tardis_data folder, which should still work as one of the check in the linked part of code below:

tardis/tardis/io/atom_data/util.py

Lines 31 to 36 in ab869ea

	fpath = os.path.join(os.path.join(get_data_dir(), fname))
	if os.path.exists(fpath):
	logger.info(
	f"\n\tAtom Data {fname} not found in local path.\n\tExists in TARDIS Data repo {fpath}"
	)
	return fpath

Could you attempt to fix those cases as well? Thanks!

Sure, why not !!!

[Sumit112192]

@DeerWhale Are the changes good?

[DeerWhale]

@DeerWhale Are the changes good?

Look great! Thanks a lot for the fix!