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

update model macro definitions for compiled code #1676

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

DougBurke
Copy link
Contributor

@DougBurke DougBurke commented Jan 18, 2023

Summary

Re-work the macro definitions used to build the compiled models and use this to clean up the handling of the XSPEC models, in particular table models. There is no functional change.

Details

Note, I have now made this PR depend on #1681 as there are some urgent test updates needed there. This has now been merged, which simplifies this PR.

PR chains that will depend on this:

I have a lot of XSPEC follow-up code and I am trying to break things up so that they are

  • easy to review
  • reduce the chance for PR collisions, or at least make it a lot easier to deal with when we do so

The change to the include files for the model macros is one of the latter.

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #1676 (5c98977) into main (0842233) will increase coverage by 0.69%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1676      +/-   ##
==========================================
+ Coverage   79.70%   80.39%   +0.69%     
==========================================
  Files          72       72              
  Lines       27159    27162       +3     
  Branches     4588     4588              
==========================================
+ Hits        21646    21837     +191     
+ Misses       5315     5133     -182     
+ Partials      198      192       -6     
Impacted Files Coverage Δ
sherpa/ui/utils.py 96.02% <0.00%> (+0.04%) ⬆️
sherpa/astro/data.py 92.61% <0.00%> (+0.05%) ⬆️
sherpa/utils/__init__.py 69.50% <0.00%> (+0.08%) ⬆️
sherpa/astro/ui/utils.py 97.36% <0.00%> (+0.11%) ⬆️
sherpa/conftest.py 92.30% <0.00%> (+0.51%) ⬆️
sherpa/plot/__init__.py 85.59% <0.00%> (+0.51%) ⬆️
sherpa/models/model.py 93.90% <0.00%> (+0.69%) ⬆️
sherpa/optmethods/optfcts.py 72.15% <0.00%> (+0.98%) ⬆️
sherpa/utils/err.py 95.65% <0.00%> (+1.08%) ⬆️
sherpa/utils/formatting.py 97.64% <0.00%> (+1.17%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0842233...5c98977. Read the comment docs.

@DougBurke DougBurke changed the title Xspec pre cleanup update model macro definitions for compiled code Jan 18, 2023
A way of indicating a Python function that uses kwargs has been
added to the top-level extension header (KWSEPC). This is then
used in the generic model-extension header, replacing the MODSPEC
definition. The more-interesting change is in the XSPEC extension
header, where again we replace the local MODSPEC definitions

a) re-order them by languange support (FORTRAN then C/C++) rather
   than the somewhat random order before (this is mainly a code
   style issue)

b) remove the table-model specific definitions as they are only
   used in one place (_xspec.cc) and we can just avoid a lot of
   complexity in the macro by hard-coding the logic. This will
   also make removing the pre XSPEC 12.10.1-specific code easier
   when we get to that PR. (this is the more interesting reason)

In a future PR I will update the XSPEC defines from using FCTSPEC
to KWSPEC, but that is left for later.
@DougBurke DougBurke added the note: easy review Indicates PR requires simple review label Jan 31, 2023
@Marie-Terrell Marie-Terrell self-requested a review February 9, 2023 20:02
Copy link
Contributor

@Marie-Terrell Marie-Terrell left a comment

Choose a reason for hiding this comment

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

Checked with a ciao-install build (xspec 12.12.1s Linux and macOS) and a Conda build (xspec 12.13.0c). All of the changes look good.

@wmclaugh wmclaugh merged commit 33905e4 into sherpa:main Feb 14, 2023
@DougBurke DougBurke deleted the xspec-pre-cleanup branch February 14, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants