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.

Sign up for GitHub

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

Jump to bottom

fix: reflect new file layout of .geom files in near_field #259

Merged
merged 3 commits into from
Jun 22, 2019
Merged

fix: reflect new file layout of .geom files in near_field #259

merged 3 commits into from
Jun 22, 2019

Conversation

timovanopstal
Copy link

@timovanopstal timovanopstal commented Jun 5, 2019 •
edited
Loading

Description

misc/near_field/gendip.f seems to read .geom files incorrectly. Corrected and tested on ./runexample

Related issues

Fixes #... or Related to #...

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the contributing guidelines
  • Code compiles correctly
  • Tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works. And these tests pass
  • I have added/extended necessary documentation (if appropriate)

Further comments

@myurkin
Copy link
Member

myurkin commented Jun 5, 2019

The fix is good. Thanks! The extra line in geometry files appeared due to recent incorporation of rectangular dipoles.
Can you additionally run the tests at RUNEXAMPLE/runall ?

@myurkin myurkin self-assigned this Jun 8, 2019
@timovanopstal
Copy link
Author

timovanopstal commented Jun 10, 2019 •
edited
Loading

Ok, I force-updated my fix. Do you mean misc/near_field/RUNTESTS/runall? In that case, now they pass. Let me know if you need anything else from me, AFAICT I've done all that needs doing here.

@myurkin
Copy link
Member

myurkin commented Jun 18, 2019

Timo, sorry for the delay. I have been at the conference previous week. I have tested your fix, and it works fine. However, I want to take this opportunity and fix a couple of related issues in near_field and combine it into this pull request. This may take a couple more days.

@myurkin
Improvements of near_field
3880cf5 
- changed processing of shape file (target) in gendip.f not to depend on precise number of commented lines. Now it also works fine for multi-domain shape files.
- updated tests, mainly due to recent ability of ADDA to use odd grids: 1) added '-grid 20' in sop to be able to compare with previous references; 2) changed references in box11 (alternatively '-grid 12' was tested as a viable fix).
- added a new test coated to test multi-domain shapes (it is incompatible with undocumented debug level 2).
- removed -ffast_math from compilation flags (caused some weird warnings).
@myurkin
Copy link
Member

myurkin commented Jun 18, 2019

Timo, I have fixed everything I wanted and pushed it to your branch (so it automatically updated this pull request). Can you please test the current state - both compiling and RUNTESTS/runall? For the latter please also check that the produced numbers are small.

@timovanopstal
Copy link
Author

I've rerun tests and they still work. As an aside, I'm on OS X with GNU Fortran from Homebrew (GCC 8.2.0) and to get things to compile, I had to

-MPIFC  = $(FC)
+MPIFC  = mpifort
 LIBS   =
-MPILIBS= -lfmpich2g

@timovanopstal
Copy link
Author

Oh, and a more important remark, you added coated/run but without execution permissions, so it didn't run with ./runall although the test does pass.

@myurkin
misc/near_field:
56e5925 
- added comments to Makefile
- added executable flags to RUNTESTS/coated/clean,run
@myurkin myurkin merged commit d6ab725 into adda-team:master Jun 22, 2019
@myurkin myurkin mentioned this pull request Jun 22, 2019
Open
@timovanopstal timovanopstal deleted the fix branch June 22, 2019 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@myurkin myurkin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timovanopstal @myurkin