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

api: Add support for subdomain allocation for Functions #1924

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

Conversation

AtilaSaraiva
Copy link
Contributor

@AtilaSaraiva AtilaSaraiva commented May 23, 2022

Hello folks,

This is the first commit for a series of commits adding support for functions to be allocated within a subdomain (let's call those subfunctions for brevity), and operated along other normal functions . This only first commit only adds allocation support for the Function class, I still need to add it for the TimeFunction class, not to mention that I still have to add support for it for the Operator class.
This is only but the first step in the right direction, or so I hope.

Cheers!

Authored by Átila Saraiva Quintela Soares (atilasaraiva@gmail.com)
SENAI CIMATEC geophysicist researcher.

Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! I've added it to my backlog of "PRs to be reviewed".

At first glance, it looks good to me. Basically, you're lifting some functionality because you need the distributor machinery to be available in the SubDomains (or MultiSubDomains), right?

devito/types/grid.py Outdated Show resolved Hide resolved
@FabioLuporini
Copy link
Contributor

so yeah I think this is going in the right direction, my other suggestion is to keep PRs as small as possible so that we also merge them as quickly as possible :)

@AtilaSaraiva
Copy link
Contributor Author

AtilaSaraiva commented May 23, 2022

so yeah I think this is going in the right direction, my other suggestion is to keep PRs as small as possible so that we also merge them as quickly as possible :)

Oh, do you mean in number of lines written, the number of commits, or number of modified files?

Thanks a lot for this! I've added it to my backlog of "PRs to be reviewed".

No problem, thanks for the early review, but I still have to fix it for TimeFunction as well.

At first glance, it looks good to me. Basically, you're lifting some functionality because you need the distributor machinery to be available in the SubDomains (or MultiSubDomains), right?

Yes that is correct, no big changes, just moving things around. I wonder if it would be more appropriate to split the CartesianDiscretization in two classes, one for the distributor machinery and other for the rest. Lemme know what you think.

@FabioLuporini
Copy link
Contributor

I wonder if it would be more appropriate to split the CartesianDiscretization in two classes, one for the distributor machinery and other for the rest. Lemme know what you think.

Mhh, what do you have in mind? Something like a top abstract class (CartesianDiscretization) and a mixin for "all the CartesianDiscretization that can 1) be used to define a (Time)Function and 2) be decomposed (with MPI) ?

@AtilaSaraiva
Copy link
Contributor Author

Mhh, what do you have in mind? Something like a top abstract class (CartesianDiscretization) and a mixin for "all the CartesianDiscretization that can 1) be used to define a (Time)Function and 2) be decomposed (with MPI) ?

yeahh, something like that, I'm not sure how methods could be exactly split but something of the sort.

@FabioLuporini FabioLuporini added the API api (symbolics, types, ...) label May 24, 2022
@FabioLuporini FabioLuporini changed the title api: Added support for subdomain allocation for Functions api: Add support for subdomain allocation for Functions May 31, 2022
@AtilaSaraiva AtilaSaraiva marked this pull request as ready for review May 31, 2022 12:55
@FabioLuporini
Copy link
Contributor

Hi @AtilaSaraiva , any news from this front?

@AtilaSaraiva
Copy link
Contributor Author

Hello Fabio! I'm studying the code for the time being, but you did say you would backport some one dimensional array functionality so I kinda need that to progress further.

Either way, I gonna need some more in depth understanding code generation and allocation to change the iteration space so Im digging the code for now.

@FabioLuporini
Copy link
Contributor

Hello Fabio! I'm studying the code for the time being, but you did say you would backport some one dimensional array functionality so I kinda need that to progress further.

ah, yes sure, in my backlog... it will take a while to get there, but we will!

Either way, I gonna need some more in depth understanding code generation and allocation to change the iteration space so Im digging the code for now.

Sure, that makes a lot of sense to me. Get in touch on slack if you have any questions (feel free to use the #development channel)

@AtilaSaraiva AtilaSaraiva force-pushed the funcSubdomain branch 2 times, most recently from 58cff8a to 34311f1 Compare July 18, 2022 14:31
@FabioLuporini
Copy link
Contributor

what's the state of this?

@AtilaSaraiva
Copy link
Contributor Author

AtilaSaraiva commented Mar 8, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API api (symbolics, types, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants