Skip to content

Replace SIGNATURE with mgard::SIGNATURE - #195

Merged
qliu21 merged 2 commits into
masterfrom
define-signature-once
Jul 5, 2022
Merged

Replace SIGNATURE with mgard::SIGNATURE#195
qliu21 merged 2 commits into
masterfrom
define-signature-once

Conversation

@ben-e-whitney

Copy link
Copy Markdown
Collaborator

This commit replaces the signature macro defined in include/mgard-x/Metadata.hpp with the mgard::SIGNATURE constant defined in include/format.hpp.

@JieyangChen7, please review this commit. In addition to the signature switch, it also removes the Metadata::signature data member (since the only valid value is mgard::SIGNATURE). I can keep that member if you prefer.

@qliu21, please don't merge this pull request yet. I'll rebase on top of fix-installation-paths once you merge #194.

@ben-e-whitneyben-e-whitney added the enhancement New feature or request label Jun 17, 2022
@ben-e-whitney

Copy link
Copy Markdown
CollaboratorAuthor

@JieyangChen7, just a reminder to take a quick look at this when you have the time.

@JieyangChen7

Copy link
Copy Markdown
Collaborator

@JieyangChen7, just a reminder to take a quick look at this when you have the time.

@ben-e-whitney Sorry about the delay. Just got back from vacation. I will review this by the end of this week.

@ben-e-whitney

Copy link
Copy Markdown
CollaboratorAuthor

No problem, @JieyangChen7. Hope you had a good vacation.

@JieyangChen7

Copy link
Copy Markdown
Collaborator

@ben-e-whitney I tried to build this PR but it gives me this error with enabling CUDA:

/home/jieyang/dev/MGARD/include/utilities.tpp:248:29: error: ‘__T288’ was not declared in this scope const CartesianProduct<T, N> &iterable,
I think this is the NVCC compiler bug we encountered before. Is there any way we can avoid including "utilities.hpp" when including "format.hpp" ?

@ben-e-whitney

Copy link
Copy Markdown
CollaboratorAuthor

@JieyangChen7 Please try again with the commit I just pushed (b6f5330). On that commit, on Summit, build_scripts/build_mgard_cuda_summit.sh ran successfully for me.

@JieyangChen7
JieyangChen7 marked this pull request as ready for review July 5, 2022 17:49
@ben-e-whitney

Copy link
Copy Markdown
CollaboratorAuthor

Thanks, @JieyangChen7. @qliu21, this is ready to be merged.

@qliu21
qliu21 merged commit 7a62897 into masterJul 5, 2022
@ben-e-whitney
ben-e-whitney deleted the define-signature-once branch July 5, 2022 18:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ben-e-whitney@JieyangChen7@qliu21