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

MP X-Macros #213

Merged
merged 24 commits into from
Jan 19, 2022
Merged

MP X-Macros #213

merged 24 commits into from
Jan 19, 2022

Conversation

bryates
Copy link
Contributor

@bryates bryates commented Oct 26, 2021

This is the MP equivalent of PR #207. It fully compiles and runs for the MP in L3PHIC using the following test vectors

memprints_url_cm="https://aryd.web.cern.ch/aryd/MemPrints_Combined_211012.tgz"
luts_url_cm="https://aryd.web.cern.ch/aryd/LUTs_Combined_211012.tgz"

without these, there are 5 events where the HLS has an extra fullmatch output.

It also adds generate_MP.py, which is needed by the CMSSW Future Emulation code.

@bryates bryates force-pushed the mp_xmacro branch 2 times, most recently from 87be434 to 2c461a1 Compare October 26, 2021 18:16
@bryates bryates requested review from aehart and carriganm95 October 26, 2021 18:53
@@ -1176,7 +1176,55 @@ X(TP_L3L4D_, "TP_L3L4D") \
X(TP_L5L6A_, "TP_L5L6A") \
X(TP_L5L6B_, "TP_L5L6B") \
X(TP_L5L6C_, "TP_L5L6C") \
X(TP_L5L6D_, "TP_L5L6D")
X(TP_L5L6D_, "TP_L5L6D") \
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I think the xmacros could be simplified to just be X(TP_L5L6D) and then generate the underscore and the text substitution directly. This is more universally true for all X-macros. @bryates @aperloff

@tomalin
Copy link
Contributor

tomalin commented Jan 12, 2022

The TC HLS module fails the CSIM CI check here, but passes it in other PRs. Does this PR need rebasing?

@bryates
Copy link
Contributor Author

bryates commented Jan 14, 2022

The TC HLS module fails the CSIM CI check here, but passes it in other PRs. Does this PR need rebasing?

I just tired to rebase with the master, but that did not fix the CI issues. @aryd could this be related to the new test vectors we're using?

@tomalin
Copy link
Contributor

tomalin commented Jan 14, 2022

The TC HLS module fails the CSIM CI check here, but passes it in other PRs. Does this PR need rebasing?

I just tired to rebase with the master, but that did not fix the CI issues. @aryd could this be related to the new test vectors we're using?

Given that PR #219 passed the CI tests yesterday, and is apparently using the same download.sh as your PR, I'm dubious.

@bryates
Copy link
Contributor Author

bryates commented Jan 14, 2022

The TC HLS module fails the CSIM CI check here, but passes it in other PRs. Does this PR need rebasing?

I just tired to rebase with the master, but that did not fix the CI issues. @aryd could this be related to the new test vectors we're using?

Given that PR #219 passed the CI tests yesterday, and is apparently using the same download.sh as your PR, I'm dubious.

This PR is using

#### test 211005/211217 ####
# Standard configuration
memprints_url="https://aryd.web.cern.ch/aryd/MemPrints_Standard_211005.tgz"
luts_url="https://aryd.web.cern.ch/aryd/LUTs_Standard_211005.tgz"
# Combined modules
memprints_url_cm="https://aryd.web.cern.ch/aryd/MemPrints_Combined_211217.tgz"
luts_url_cm="https://aryd.web.cern.ch/aryd/LUTs_Combined_211217.tgz"

while #219 is using
#### fw_synch_210611 ####
# Standard configuration
memprints_url="https://cernbox.cern.ch/index.php/s/hUJUsqvCnKv2YdQ/download"
luts_url="https://cernbox.cern.ch/index.php/s/9Yms3LCKJsg7UmF/download"
# Combined modules
memprints_url_cm="https://cernbox.cern.ch/index.php/s/RFpmFiSnFC84x0O/download"
luts_url_cm="https://cernbox.cern.ch/index.php/s/kqZu8R7Ftu0YPoO/download"

@aehart
Copy link
Contributor

aehart commented Jan 18, 2022

The TC HLS module fails the CSIM CI check here, but passes it in other PRs. Does this PR need rebasing?

I just tired to rebase with the master, but that did not fix the CI issues. @aryd could this be related to the new test vectors we're using?

Given that PR #219 passed the CI tests yesterday, and is apparently using the same download.sh as your PR, I'm dubious.

This PR is using

#### test 211005/211217 ####
# Standard configuration
memprints_url="https://aryd.web.cern.ch/aryd/MemPrints_Standard_211005.tgz"
luts_url="https://aryd.web.cern.ch/aryd/LUTs_Standard_211005.tgz"
# Combined modules
memprints_url_cm="https://aryd.web.cern.ch/aryd/MemPrints_Combined_211217.tgz"
luts_url_cm="https://aryd.web.cern.ch/aryd/LUTs_Combined_211217.tgz"

while #219 is using

#### fw_synch_210611 ####
# Standard configuration
memprints_url="https://cernbox.cern.ch/index.php/s/hUJUsqvCnKv2YdQ/download"
luts_url="https://cernbox.cern.ch/index.php/s/9Yms3LCKJsg7UmF/download"
# Combined modules
memprints_url_cm="https://cernbox.cern.ch/index.php/s/RFpmFiSnFC84x0O/download"
luts_url_cm="https://cernbox.cern.ch/index.php/s/kqZu8R7Ftu0YPoO/download"

I'm not sure about the origin of the new test vectors, but in any case, I think we want to leave the URLs in download.sh alone in this PR. The test vectors are properly updated in #220.

@bryates
Copy link
Contributor Author

bryates commented Jan 18, 2022

@aehart I've commented out the new test vectors, and set the CI to allow the MP to fail again. This PR should be good to go.

Copy link
Contributor

@aehart aehart left a comment

Choose a reason for hiding this comment

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

Looks good. Approved!

@aehart aehart merged commit 478efa1 into master Jan 19, 2022
@aehart aehart deleted the mp_xmacro branch January 19, 2022 14:59
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.

5 participants