-
Notifications
You must be signed in to change notification settings - Fork 3
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 creating solver interface #35
Conversation
musica-fortran/src/micm/MICM_mod.F90
Outdated
subroutine delete_micm_polymorph(this) | ||
implicit none | ||
class(micm) :: this | ||
call delete_micm_c(this%ptr) | ||
end subroutine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that yes, we need this. I also had the same question and without it, it doesn't work. It needs to declare micm as class
instead of type
to be able to have access to its members. I found this answer useful to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated it to class
and removed type
version.
f159a5e
to
80f0858
Compare
musica-fortran/src/micm/micm_mod.F90
Outdated
private | ||
type(c_ptr) :: ptr | ||
contains | ||
procedure :: delete => delete_micm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattldawson is this a time when finalize
could/should be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think so - if this function is always called right before the object is deleted, it could be:
procedure :: delete => delete_micm | |
final :: delete_micm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function itself will have to be modified to take a type(micm_t)
instead of a class(micm_t)
with intent(inout)
, but other than that should be mostly the same (I think). This final
function gets called automatically when a micm_t
object goes out of scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be okay to leave the way it is then? It needs to be class(micm_t)
for micm_t
to have access to its members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should still have access to the data members, you just would need to change it to:
subroutine delete_micm(this)
type(micm_t), intent(inout) :: this
call delete_micm_c(this%ptr)
end subroutine
I think the only difference between class(foo_t)
and type(foo_t)
is that class(foo_t)
will accept types that extend foo_t
, but type(foo_t)
won't, and that type-bound procedures have to take the polymorphic class
argument, but final routines need to take the concrete type
argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you for correcting me. I fixed it
musica-fortran/src/micm/micm_mod.F90
Outdated
|
||
contains | ||
function create_micm(config_path) | ||
implicit none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please place implicit none
at the top of this on the line before private
and remove implicit none
in all of these functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's done now.
musica/src/micm/micm.cpp
Outdated
return success; | ||
} | ||
|
||
void MICM::delete_solver() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a destructor so that in delete_micm
you can just call delete micm
and not have to care about calling this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I updated the destructor.
void solve(double temperature, double pressure, double time_step, double*& concentrations, size_t num_concentrations); | ||
|
||
private: | ||
static constexpr size_t NUM_GRID_CELLS = 1; // TODO(jiwon) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can set up a separate issue for supporting multiple grid cells
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! I don't see anything other than what Kyle commented on
Completed