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

Setup.py for easier library compilation, and installation is streamli… #7

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

btasdelen
Copy link
Collaborator

…ned and documented. Unnecessary build and binary files are removed.

…ned and documented. Unnecessary build and binary files are removed.
@btasdelen btasdelen requested a review from pkash16 June 12, 2024 04:07
@pkash16
Copy link
Contributor

pkash16 commented Jun 12, 2024

I made a few minor fixes, namely that the example config was out of date and we need to make sure directories exist before writing to them.

There is a bigger issue, however, that I haven't been able to resolve.

Somehow, seq.signature_value is empty after seq.write(). When following the breakpoint step by step, it seems that signature value is correctly written in pypulseq->Sequence->write_seq. However, when the scope leaves the write_seq function, self.signature_value becomes an empty string again (not sure why). This could be a pypulseq issue, but it makes our file name for trajectory be an empty string.

@pkash16
Copy link
Contributor

pkash16 commented Jun 12, 2024

Other than that, the compilation was very easy and smooth, and it just worked by following the instructions, which is great!

@pkash16
Copy link
Contributor

pkash16 commented Jun 12, 2024

OK I figured out this is a problem with a latest PR in PyPulseq: imr-framework/pypulseq#178

This commit seems to be the issue. In write_seq (line 40), I believe should be
self = self.remove_duplicates(in_place=True) intead of self = self.remove_duplicates() because it messes with the python reference of 'self' in later lines of code. Maybe you can resolve this in that repository. This PR looks great and I can merge.

@pkash16 pkash16 merged commit 0f00568 into main Jun 12, 2024
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.

2 participants