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

Improve formatting of f90 code in classic. #643

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

ketch
Copy link
Member

@ketch ketch commented Apr 27, 2020

The main purpose of this was to get rid of labeled do loops, since this throws a warning with recent gfortran compilers. I also improved indentation and comment formatting.

@rjleveque
Copy link
Member

Nice, thanks!!

I'll take a closer look and test it all soon.

We should do the same in amrclaw and geoclaw eventually.

@mandli
Copy link
Member

mandli commented Apr 27, 2020

This is raised in clawpack/geoclaw#425 and clawpack/amrclaw#247 as good first issues.

@ketch
Copy link
Member Author

ketch commented Apr 27, 2020

It's a fairly error-prone process, so may not actually be a good first issue.

There are still a number of improvements that could be made (some line continuations that aren't necessary, some comment indentation I didn't get to fixing, etc.) but I ran out of steam. I think the most important things are here and the warnings from f2py should be much fewer.

@mandli
Copy link
Member

mandli commented Apr 27, 2020

Agreed, some of the routines in AMRClaw and GeoClaw will be difficult.

This looks good to me and works. Thanks for also cleaning up those comments and doing some alignment fixes.

@ketch
Copy link
Member Author

ketch commented Apr 27, 2020

Don't merge yet -- I realized I forgot a couple of files. Give me 5 minutes.

@ketch
Copy link
Member Author

ketch commented Apr 27, 2020

Okay, there are probably more files that still need this treatment but now I at least got the ones in the sw_sphere directory where @rjleveque was seeing a segfault. This probably won't fix that, but fingers are crossed.

@rjleveque
Copy link
Member

@ketch - sorry for being slow to follow up. I just tried this again and still get the segfault in pyclaw/examples/shallow_sphere:

$ make all   # works fine
$ python test_shallow_sphere.py 
Segmentation fault: 11

@mandli
Copy link
Member

mandli commented Jan 16, 2021

Another thought for this. I just noticed that gfortran includes -fimplicit-none debug flag, which would help to start to remove implicit definitions. This actually might be something someone could do somewhat straight forwardly and help with the f2py interface as well.

@ketch
Copy link
Member Author

ketch commented Jan 17, 2021

@rjleveque So the shallow sphere test runs fine for you with the master branch, but segfaults with this PR branch?

And this happens when each is run inside a fresh install? Because my best guess would be that you have some compiled files from master still hanging around when you run the PR. It's hard to see what else could cause this since the only changes are loop numbers and indentation (I just checked everything carefully again).

@rjleveque
Copy link
Member

It also gives a seg fault when run with the master branch (and redoing make all), so it is probably a gfortran compiler issue not specific to this PR.

@ketch
Copy link
Member Author

ketch commented Jan 17, 2021

In that case, since it passes on Travis I would vote to go ahead and merge.

@rjleveque rjleveque merged commit feab1b5 into clawpack:master Jan 17, 2021
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