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

Progress in refactoring code and implementing unit tests #82

Draft
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

kousuke-nakano
Copy link
Collaborator

@kousuke-nakano kousuke-nakano commented Sep 26, 2023

What I am doing right now are:

  • Adding intent(in, out, inout) statements,
  • Adding use only statements, and
  • Switching the bracket convention to the star convention in the declaration (e.g. real(8) -> real*8).
  • Implementing unit tests for the updated codes if possible.

In this pull request, I have changed several codes in /src/m_common/

  • WIP
  • IntelOneAPI installation did not properly work (temporarily).

#12

@kousuke-nakano kousuke-nakano added the refactoring Refactoring code label Sep 26, 2023
@kousuke-nakano kousuke-nakano self-assigned this Sep 26, 2023
Copy link
Collaborator

@addman2 addman2 left a comment

Choose a reason for hiding this comment

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

I have a few suggestions

logical symmagp
! argument parameters
integer, intent(in) :: ndim, ind, ipf
real(8), intent(in) :: value
Copy link
Collaborator

Choose a reason for hiding this comment

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

real(8) and real*8 is not the same. Well it is in this case but not in the complex case. Its good to stick with one form. In most of the core star convetion is use.

Copy link
Collaborator Author

@kousuke-nakano kousuke-nakano Sep 26, 2023

Choose a reason for hiding this comment

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

Hi @addman2 Thank you for the suggestion! I used the same conventions as the old ones. I agree to stick with one form! Shall we use the core star convention (real*8, complex*16 etc...) in all files? I can do this change while refactoring the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @kousuke-nakano ,

Yes, I think the star convention is already used more than the bracket convention. So to change to star one is easier than other way around. I can add this to the linter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dear @addman2, sure! I will use the star convention in all sources. I will fix it during the ongoing refactoring process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dear @kousuke-nakano,

what is the problem with intel OneAPI compilation? I know there are issues with it which one you experience?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HI @addman2, the problem was the Intel GPG key. I guess it was updated. The problem has been solved. see #105


! argument parameters
integer nion
real(8), intent(in) :: rkel(3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, see comentary of upsim.f90

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same above.


! argument parameters
integer, intent(in) :: nion
real(8), intent(in) :: zeta(nion), iond(nion, nion), kappa, LBox
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same above

@michele-casula
Copy link
Collaborator

michele-casula commented Oct 12, 2023 via email

@kousuke-nakano
Copy link
Collaborator Author

Dear @michele-casula, yes, you are right. The bracket one seems newer. Dear @addman2, we can use the bracket one, what do you think? As you pointed out, we should be careful about the complex case, i.e., complex(8) = complex*16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants