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

we're on python3, so importing from future should no longer be necessary #726

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

pavelkomarov
Copy link
Contributor

Just a small PR. I might PR this repo more as I use it.

Copy link
Contributor Author

@pavelkomarov pavelkomarov left a comment

Choose a reason for hiding this comment

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

A lot of the fortran codes are .f90 files, yet here we're using fc.compiler_f77. That won't cause any issues? Is a fortran 1990 compiler available that we should be using instead?

@mandli
Copy link
Member

mandli commented Sep 24, 2024

I think for legacy support fc.compiler_f77 defaults to gfortran, which can handle both formats, usually anyway. I am not even sure that f77 compilers exist on anything modern anyway.

@mandli
Copy link
Member

mandli commented Sep 24, 2024

This is failing because of the shallow water test, which I think was failing before.

@pavelkomarov
Copy link
Contributor Author

How can we fix that test? Should we replace mention of '77 with a function named for gfortran? I bet one exists.

@mandli
Copy link
Member

mandli commented Sep 24, 2024

I think the compiler mentioned should be fine and I am not certain what consequences it would have to change to a f90 version.

@ketch do you remember why that test is failing?

@pavelkomarov
Copy link
Contributor Author

According to the numpy docs, distutils is legacy and may be removed in a future version https://numpy.org/doc/stable/f2py/buildtools/distutils.html

@ketch ketch mentioned this pull request Sep 25, 2024
@ketch
Copy link
Member

ketch commented Sep 25, 2024

Yes, we can certainly remove those imports. But I think it would be best to do it all in one PR; otherwise we will clutter the history with dozen's of PRs just for this tiny change.

@ketch
Copy link
Member

ketch commented Sep 25, 2024

The failing test is discussed here: #642. Let's keep the discussion of it there so it's easier to follow.

@pavelkomarov
Copy link
Contributor Author

Tried to squash commits and created three commits.

@ketch
Copy link
Member

ketch commented Sep 25, 2024

Thanks, that's great!

It looks like this is failing, and I'm guessing it's related to an absolute vs. relative import issue, but I don't remember what actually changes by not importing absolute_import. The actual error occurs during the build stage and is

../../pyclaw/src/petclaw/meson.build:37:5: ERROR: File cfl.py does not exist.

The file cfl.py does exist in that directory. I don't think the issue is with that file specifically (it is the first file, alphabetically, in the first subdirectory, alphabetically). Rather, this seems to have made it so meson can't find any of the source files.

@pavelkomarov
Copy link
Contributor Author

Okay, any idea how I fix that? I don't think meson should be relying on importing from future. Do we need to do that import .package relative import kinds of constructions or something?

@ketch
Copy link
Member

ketch commented Sep 26, 2024

I spent a half hour looking into it but I still have no idea how to fix it.

@pavelkomarov
Copy link
Contributor Author

If the build is this fragile, it will be difficult to make contributions.

@pavelkomarov
Copy link
Contributor Author

Somehow I managed to delete a whole file there. Hopefully that was the problem.

@ketch
Copy link
Member

ketch commented Oct 3, 2024

Haha; of course that was the problem.

@ketch ketch merged commit 22403c8 into clawpack:master Oct 3, 2024
1 of 2 checks passed
@pavelkomarov pavelkomarov deleted the pavels_edits branch October 7, 2024 22:36
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.

3 participants