Skip to content

Conversation

@sergisiso
Copy link
Collaborator

@sergisiso sergisiso commented Apr 7, 2025

Re-enable LFRic extraction by moving the Extraction Driver Creation to the extract_node lowering step

arporter and others added 30 commits March 10, 2025 15:26
…ion do not have to deal with them, also convert pointers to allocatables in the driver
@sergisiso
Copy link
Collaborator Author

@hiker CI-permitting, this is ready for another look

@hiker
Copy link
Collaborator

hiker commented Jun 25, 2025

Regarding #2918 (comment) I have implemented a lightweight version of this: I took your suggestion of splitting the expected assert in two, with the second one containing the "read-only" fields that could be deleted in the future. But I have only done this to the end section (the compare for the driver, and the ProvideVariable for the extraction). The information about what should be kept or removed is already preserved, but the future PR will have to amend the declarations/set-up steps. I think that was easier and produces more clear asserts than creating branches for the things that are different and a lot of xfail test in the extraction. Let me know what you think.

That's good, thanks a lot!

Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

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

I'm posting this now. I just realised that my local git is corrupt, and I think I didn't get the right version of this branch (I get some errors that were apparently resolved).

@hiker
Copy link
Collaborator

hiker commented Jun 26, 2025

OK, after a clean checkout and no more corruptions: examples/gocean/eg5/extract does not work, the driver still call the kernel before reading in the data:

  do j = d_fld_whole_ystart, d_fld_whole_ystop, 1
    do i = d_fld_whole_xstart, d_fld_whole_xstop, 1
      call init_field_code(i, j, d_fld_data, 4.0)
    enddo
  enddo
  call extract_psy_data%OpenReadModuleRegion('main', 'init')
  call extract_psy_data%ReadVariable('a_fld_data', a_fld_data)

Did you perhaps forgot to push your latest version? Back to you for now.

@sergisiso
Copy link
Collaborator Author

examples/gocean/eg5/extract does not work

Aha, the previous time it was the same error but in example/lfric/eg11, the problem is that src/psyclone/domain/common/extract_driver_creator.py and src/psyclone/domain/lfric/lfric_extract_driver_creator.py have very similar code but I only fixed the issue (change the order of some statements) in one of them.

Ideally, we should merge them all in a single class, which I think now is possible, but resisted doing it in this PR as it is already very big.

@sergisiso
Copy link
Collaborator Author

@hiker I fixed the latest issues, this is ready to continue the review

Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

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

All changes look good. I did some tests with extraction of kernels in gravity wave, and they all worked.

I will trigger the CI tomorrow, and then merge this in. @sergisiso , could you just bring this branch up-to-date with master (just in case that there are any conflicts)? Thanks

@sergisiso
Copy link
Collaborator Author

Thanks @hiker. I brought it to master and tiggered the CI action myself, as the machine is likely to be busy and may take a while to finish.

@hiker
Copy link
Collaborator

hiker commented Jul 3, 2025

CI was all green, will merge once github CIs is happy.

@hiker hiker merged commit c2ebc91 into master Jul 3, 2025
11 checks passed
@hiker hiker deleted the lfric_extraction_at_lowering branch July 3, 2025 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants