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

Apply fixes for the CPC submission #1

Merged
merged 5 commits into from
Nov 6, 2018

Conversation

cffischer
Copy link
Member

Correction of errors found after submission to CPC:

  1. in RCI90_mpi : The rci90mpi.f90 routine opened the .clog file again after it had already been opened.
    The second open and associated write commands were commented out.

  2. RMCDHF90_mpi: there were two corrections:
    i) getscd.f90 and getscd_I.f90 were deleted since they were not used.
    ii) The statement
    RNT = 2.D-6
    was replaced by
    !CFF ... RNT should be Z-dependent
    RNT = 2.0D-06/Z
    With this change, the grid for the hydrogen-like equation is a transformation of the hydrogen grid.
    With these changes, this version is equivalent to the CPC version of grasp2018.

@cffischer
Copy link
Member Author

This repository is now equivalent to the latest CPC submission of the Grasp2018 and is ready for distribution as a public repository.

@mortenpi
Copy link
Member

2. ii) The statement
RNT = 2.D-6
was replaced by
!CFF ... RNT should be Z-dependent
RNT = 2.0D-06/Z
With this change, the grid for the hydrogen-like equation is a transformation of the hydrogen grid.

I don't see this change. The PR currently only touches src/appl/rci90_mpi/rci90mpi.f90 and src/appl/rmcdhf90_mpi/getscd.f90

With these changes, this version is equivalent to the CPC version of grasp2018.

So the code has already been submitted to CPC? In any case, I will then make the repo public as soon as this is merged.

@mortenpi mortenpi changed the title cff/fix-error1 Fix minor errors in rmcdhf and rci Oct 15, 2018
@cffischer
Copy link
Member Author

cffischer commented Oct 15, 2018 via email

1. Deletes src/appl/rmcdhf90_mpi/getscd_I.f90  (not needed)
2. Makes RNt in the numerical grid Z-dependent in rmcdhf90_mpi
3. Corrects some maneig errors when using the Intel libraries
! 2 Davidson, Dense-Memory => dense requires less memory *
! 3 Davidson, Sparse-Memory => sparse requires less memory *
! 4 Davidson, Sparse-Disk => memory requirement too large *
! for at least one process *
Copy link
Member

Choose a reason for hiding this comment

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

Is the removal of these comments intentional? I suppose the NELMNT part is no longer needed, but the other ones are useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

#1 I could not find those comments in the original tarball. The comments pertain to an old version of rci90 where the user specified how the eigenvalue is determined. Now this is done according to memory available and memory needed. It was not evident on my local machine. So why would it appear on a merge?

@mortenpi
Copy link
Member

Here are the steps to clone this PR to your computer. As the branch being PRd lives on a fork, it is slightly non-trivial:

# Clone the CompAS repository, if you already haven't
git clone https://github.com/compas/grasp2018.git
# do this instead if you have SSH keys set up:
#git clone git@github.com:compas/grasp2018.git

# cd into your local repository
cd grasp2018

# Add the fork as a separate Git remote
git remote add cffischer https://github.com/cffischer/grasp2018.git
# do this instead if you have SSH keys set up:
#git remote add cffischer git@github.com:cffischer/grasp2018.git 

# Fetch the contents of the forked repository
git fetch cffischer

# Checkout the PR branch
git checkout cff/fix-error1

# After this your worktree (i.e. the files on the disk) should correspond
# to the code in the pull request. Check this by running
git status
git log
# git status should say "On branch cff/fix-error1" and the log should show
# the relevant commits.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

The rmcdf_mpi and rci_mpi programs appear to compile and run. I did not check the correctness of the outputs though.

@cffischer
Copy link
Member Author

Intel is extremely particular about adhering to a standard. Gediminas said rci90 and rci90_mpi both had “bugs”. Then he sent me a maneig.f90 for rci90_mpi and somehow I came to the conclusion that maneig.f90’s were the same. Actually, I believe he sent me the corrected version for rci90 because the parallel version needs to have the number of processor (his version had nprocs=1) and needs to limit the output so that the same thing is not printed 100 times when running a large job on 100 processors. So I need to sort this out with Gediminas.

There are lots of irrelevant comments in GRASP. Gediminas (and others) have the habit of commenting out lines but never removing any. So after a while the code is not very readable. In this case a procedure had changed but the comments remain.

Sorry. It would have been nice to get this finished. Besides, I forget things after a while.

with issues using the Intel libraries for Lapack.
@cffischer
Copy link
Member Author

tool/lscomp.pl should now be executable

@cffischer
Copy link
Member Author

Changing the permission of lscomp.pl to executable

@cffischer
Copy link
Member Author

I think we have code that is "perfect" for CPC, although more confirmations would be helpful.

@mortenpi mortenpi changed the title Fix minor errors in rmcdhf and rci Apply fixes for the CPC submission Nov 6, 2018
@mortenpi mortenpi merged commit 92021fc into compas:master Nov 6, 2018
mortenpi added a commit that referenced this pull request Jan 28, 2020
Remove custom Doxygen repository as it is no longer working. Doxygen in the official repositories is slightly older but should be sufficient for GRASP.
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.

2 participants