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

Added diffusion and angle distribution #159

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

davytorres
Copy link
Collaborator

No description provided.

@davytorres davytorres requested review from rouson and Dominick99 June 27, 2023 07:23
Copy link
Collaborator

@rouson rouson left a comment

Choose a reason for hiding this comment

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

In addition to the requested changes, please also either remove commented code or make the code conditional so that setting some parameter will actually execute it. In an ideal world, there would also be a test that sets that parameter to make sure the code actually works.

integer, intent(in) :: ndim
integer, intent(in) :: ng(:)
double precision, intent(in) :: gb,ge
double precision gridp(2*ndim+1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@davytorres because gridp is not a dummy argument, please move it to the grid_s submodule, make it allocatable and then allocate it there.

double precision gridp(2*ndim+1)
end function gridparameters

module function gradient(self, my_num_cells, ng, dx, gb, tconc, x, concentration_subgrid) result(gx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@davytorres it appears likely that this procedure could be pure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because concentration_subgrid is modified inside the procedure, making gradient pure will also require converting it to a subroutine and giving concentration_subgrid the attribute intent(inout).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried making the changes but the co_sum call cannot be used in a pure procedure.

@@ -34,11 +35,21 @@ pure module function construct(input, history) result(output)
interface

module function simulated_distribution(self) result(output_distribution)
!result(output_distribution,output_angle_distribution)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove commented code.

end function

module function simulated_distribution_angles(self) result(output_angle_distribution)
!result(output_distribution,output_angle_distribution)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove commented code.

@davytorres davytorres marked this pull request as ready for review July 12, 2023 07:15
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