Skip to content

Remove resonance reconstruction and Cython dependency - #3111

Merged
shimwell merged 6 commits into
openmc-dev:developfrom
shimwell:removing_reconstruct_resonance_cython5
Aug 14, 2024
Merged

Remove resonance reconstruction and Cython dependency#3111
shimwell merged 6 commits into
openmc-dev:developfrom
shimwell:removing_reconstruct_resonance_cython5

Conversation

@jon-proximafusion

Copy link
Copy Markdown
Contributor

Description

This PR aims to remove the reconstruct.pyx file and associated usage.

If I remember the correctly then this file was made before njoy was opensource and openmc performed some nuclear data processes

Now njoy is open source this functionality is no longer needed.

If both this PR and PR #3101 are successful then this helps making the pypi pip packaging easier as we can remove the cython from the code.

Checklist

  • I have performed a self-review of my own code
  • I have followed the style guidelines for Python source files (if applicable)

@HunterBelanger

Copy link
Copy Markdown
Contributor

Are any of the features in reconstruct.pyx in Paul's new ENDF package, or is the idea to simply remove this functionality completely ? Because if all the cython is removed from our Python API, that will definitely simply some of the workarounds I had to add to #2919 to make things work.

@shimwell

Copy link
Copy Markdown
Member

ah sorry I should have added some more context here. I saw Paul at the open source workshop last week and asked what were the options for these files. They are not in Pauls endf package. However the files were added when njoy was not open source and now that njoy is open we don't need these files anymore. They don't get much usage in the code.

@paulromano

Copy link
Copy Markdown
Contributor

To add some more context here — the Cython code in question that performs resonance reconstruction was added (#721) at a time when NJOY was not yet open sourced and there was a thought that perhaps we would do all our nuclear data processing "in house". However, shortly after this was merged, NJOY became open source and thus the resonance reconstruction in OpenMC sort of lost its raison d'être. It's also worth noting that what we have in OpenMC is also incomplete (doesn't handle LRF=7) and sometimes incorrect (see original description in #721). So, with that being said, I am not at all opposed to chopping this block of code and removing our dependence on Cython.

@paulromano
paulromano requested a review from shimwell as a code owner August 14, 2024 13:00
@paulromano paulromano changed the title Removing reconstruct resonance cython Remove resonance reconstruction and Cython dependency Aug 14, 2024

@shimwell shimwell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

super super, this will be really helpful, thanks for finishing the PR Paul

Congrats on your first PR @jon-proximafusion 😄 hope it will be the first of many

Will merge when tests pass

@shimwell
shimwell enabled auto-merge (squash) August 14, 2024 13:19

@paulromano paulromano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking care of this @jon-proximafusion!

@shimwell
shimwell merged commit 9483cce into openmc-dev:develop Aug 14, 2024
apingegno pushed a commit to apingegno/openmc that referenced this pull request May 7, 2026
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
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.

4 participants