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

Create structures for list arrays and Add OpenACC data transfer directives #249

Merged
merged 12 commits into from
Oct 25, 2023

Conversation

mlee03
Copy link

@mlee03 mlee03 commented Oct 11, 2023

In this PR,

  • structure Minmaxavglists has been declared to hold all "list" arrays pertaining to the output grid
  • function get_minmaxavg_lists has been created. This function computes values for the "list" arrays in structure Minmaxavglists
  • OpenACC directives to transfer data to/from the device has been added.

tools/fregrid/globals.h Outdated Show resolved Hide resolved
Copy link
Contributor

@ngs333 ngs333 left a comment

Choose a reason for hiding this comment

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

I added a few comments. We might need a brief meeting to discuss some of them.

tools/libfrencutils/create_xgrid_util.c Outdated Show resolved Hide resolved
++nxgrid;
if(nxgrid > MAXXGRID) error_handler("nxgrid is greater than MAXXGRID, increase MAXXGRID");
}
if(xarea/min_area > AREA_RATIO_THRESH ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Which editor or IDE are you using and is it it able to use the file FRE-NCTools/.editorconfig ? (PS Emacs and VSCode should be able to use it) I am not sure, but either :
a) your editor may be breaking the indentation of some of these lines.
b) we may have to upgrade file .editorconfig
c) This file (and a few others) was never formatted post the intro of .editorconfig in the project.
PS If you do decide to apply whole file reformatting, we may not be able to do so for the "usage" comment near
the top.
PPS This also applies to other files (or functions) that were worked on.

Copy link
Author

Choose a reason for hiding this comment

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

Why am I having these spacing issues all of a sudden? Is this .editorconfig file new?

Copy link
Author

Choose a reason for hiding this comment

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

I did download the editorconfig plugin for emacs but these mysterious differences are still showing up

Copy link
Contributor

Choose a reason for hiding this comment

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

@mlee03
OK I'll accept it. Relating to this we've had a historical and informal modus operandi you might want to be aware of. With the intent of helping the reviewer, usually we have not reformatted entire files, but only those functions that were needed to be modified to fix the bug or add the enhancement. This ultimately stemmed from the fact that we have limited unit tests, and so reviewers must look carefully at those lines that are truly modified or changed; looking at other lines is extra work and effort that detracts from checking correctness. If it was desirable to reformat entire files just to upgrade the format, this was done in a separate push and possibly with a different PR.

@mlee03
Copy link
Author

mlee03 commented Oct 19, 2023

@ngs333, ready for merging with your approval.

Copy link
Contributor

@ngs333 ngs333 left a comment

Choose a reason for hiding this comment

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

Approving with two comments left elsewhere.

void malloc_minmaxavg_lists(const int n,
double **lon_min_list, double **lon_max_list, double **lat_min_list, double **lat_max_list,
int **n_list, double **lon_avg, double **lon_list, double **lat_list)
void malloc_minmaxavg_lists(const int n, Minmaxavg_lists *minmaxavg_lists)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

OK This new version could work.

++nxgrid;
if(nxgrid > MAXXGRID) error_handler("nxgrid is greater than MAXXGRID, increase MAXXGRID");
}
if(xarea/min_area > AREA_RATIO_THRESH ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mlee03
OK I'll accept it. Relating to this we've had a historical and informal modus operandi you might want to be aware of. With the intent of helping the reviewer, usually we have not reformatted entire files, but only those functions that were needed to be modified to fix the bug or add the enhancement. This ultimately stemmed from the fact that we have limited unit tests, and so reviewers must look carefully at those lines that are truly modified or changed; looking at other lines is extra work and effort that detracts from checking correctness. If it was desirable to reformat entire files just to upgrade the format, this was done in a separate push and possibly with a different PR.

@ngs333 ngs333 merged commit af6c817 into NOAA-GFDL:gpu_dev Oct 25, 2023
4 checks passed
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