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

Runtime Type check bug in utils.py #367

Closed
DanielCalero1 opened this issue Jul 8, 2024 · 6 comments
Closed

Runtime Type check bug in utils.py #367

DanielCalero1 opened this issue Jul 8, 2024 · 6 comments
Assignees
Labels
Milestone

Comments

@DanielCalero1
Copy link

Hi, I've been using iodata lately to extract the electron integrals from a fcidump file and just today the code that I've been using for some time failed.

The line of code that is failing is

data = fcidump.load_one(open("h4_2_0.0_STO-6G.fcidump"))

with the error "Types of file and lineno are not supported: <_io.TextIOWrapper name='h4_2_0.0_STO-6G.fcidump' mode='r' encoding='UTF-8'>, None"

I spent some time looking through the module code and I think I found the source of the error.
When a loadWarning is raised inside the function "fcidump.load_one" the function "_interpret_file_lineno" of utils.py is called. Within the following lines I found the problem:

...
if isinstance(file, TextIO):
        return file.name, lineno
    if file is None:
        if lineno is not None:
            raise TypeError("A line number without a file is not supported.")
        return None, None
    raise TypeError(f"Types of file and lineno are not supported: {file}, {lineno}")

The problem is, as far as I know, that the class TextIO can not be used for checking data types at run time. Instead, it can be changed for

import io
...
if isinstance(file, io.TextIOBase): 
        return file.name, lineno
    if file is None:
        if lineno is not None:
            raise TypeError("A line number without a file is not supported.")
        return None, None
    raise TypeError(f"Types of file and lineno are not supported: {file}, {lineno}")
...

io.TextIOBase is the base class for all text file objects in Python, and it can surely be used for Runtime type checks

@tovrstra
Copy link
Member

tovrstra commented Jul 9, 2024

Thanks for brining up this problem, giving a clear example and analyzing the issue!

The canonical way to open your file with IOData, would be as follows:

from iodata import load_one
data = load_one("h4_2_0.0_STO-6G.fcidump", fmt="fcidump")

Can you try if that works?

The fact that your example used to work, is somewhat a coincidence. The argument of the file-format specific load_one functions is expected to be a LineIterator instance, which is created in the generic load_one function. LineIterator has an API that is similar to TextIOBase, but it also adds a few extras, but most file formats (including fcidump) do not need the extras.

Things to be done:

  • In any case, I agree that it would be better to change the type checking as you suggest. See Fix typecheck: TextIOBase instead of TextIO #368

  • We can add a "*.fcicdump" pattern to the fcidump format, so you don't need to specify the format as in my example. See Add support .fcidump extension #370. After adding the pattern, the following will also work:

    from iodata import load_one
    data = load_one("h4_2_0.0_STO-6G.fcidump")

@DanielCalero1
Copy link
Author

Hi, thank you for addressing this issue so promptly.

I tried the option you mentioned, but it is not working. I got the error "TypeError: load_one() got an unexpected keyword argument 'fmt'"

@tovrstra
Copy link
Member

tovrstra commented Jul 9, 2024

Is there a chance that the TypeError is obtained with a different load_one function, e.g. the one from iodata.fromats.fcidump? I was referring to this one:

https://github.com/theochem/iodata/blob/main/iodata/api.py#L158

It is made available as from iodata import load_one by the following line:

https://github.com/theochem/iodata/blob/main/iodata/__init__.py#L28

@DanielCalero1
Copy link
Author

Oh I see, yeah, that works fine!

Thank you for your help

@tovrstra
Copy link
Member

tovrstra commented Jul 9, 2024

@DanielCalero1 Thanks for verifying. The fixed type checking just got merged. This means your example should work with the latest revision, even if it is not the documented way of using it.

@tovrstra
Copy link
Member

tovrstra commented Jul 9, 2024

Fixed in #368 and #370

@tovrstra tovrstra closed this as completed Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants