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

FIX: Bug when CMPLX macro undefined. #620

Merged

Conversation

RemingtonRohel
Copy link

Targeting #619

NOTE: This branch has no conflicts with the main branch, so the target branch for this PR could be changed if this change would be better suited as a hotfix.

Changes

  • Replaced complex number instantiation using CMPLX(real, imag) with real + imag * I as per https://stackoverflow.com/a/9860772
  • Specified double precision for complex numbers in rylm.c
  • Simplified complex number instantiation/multiplication in some places. Previously, some calculations were extracting the real and imaginary parts separately and using them in a CMPLX() call to create a new complex number, rather than simply using the already-complex number.
  • Answer at https://stackoverflow.com/a/59597456 explains that CMPLX is missing on MacOS when compiling with gcc because the responsibility of defining macros like CMPLX is not up to gcc, so it truly is not defined anywhere. This is not a linker issue nor an issue with the complex.h header file missing/not found/wrong.

* Specified double precision for complex numbers in rylm.c
* Simplified complex number instantiation/multiplication in some places. Previously, some calculations were extracting the real and imaginary parts separately and using them in a CMPLX() call to create a new complex number, rather than simply using the already-complex number.
* Answer at https://stackoverflow.com/a/59597456 explains that CMPLX is missing on MacOS when compiling with gcc because the responsibility of defining macros like CMPLX is not up to gcc.
@RemingtonRohel
Copy link
Author

I believe this also would address the problems of #475

@aburrell
Copy link
Contributor

I just tried compiling this branch on my computer. This does resolve the COMPLEX problem I encountered previously. However, I ran into the flag issue reported in #609. I tried the recent suggestion of removing the -D_DARWIN flag and that helped me get past istp_plot.1.11. However removing the -D_DARWIN flag from the makebin and makelib files causes map.1.18 (an potentially others after this) to fail.

@RemingtonRohel
Copy link
Author

I added another commit to remove the effect of the -D_DARWIN flag in istp_plot.1.11 and other instances where it was used in the same way. @aburrell does it compile now for you? If not, could you include the output when compiling?

Copy link
Contributor

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

This solves my compilation issues on OS X Sonoma.

@aburrell
Copy link
Contributor

aburrell commented Oct 2, 2024

@RemingtonRohel happy to merge or to allow another person to test.

@RemingtonRohel
Copy link
Author

I'm not a maintainer, so I can't merge this myself. If it's fine with the normal RST workflow, by all means go ahead!

@pasha-ponomarenko pasha-ponomarenko merged commit 375b183 into SuperDARN:develop Oct 2, 2024
1 of 2 checks passed
@pasha-ponomarenko
Copy link
Contributor

Merged.

@RemingtonRohel RemingtonRohel deleted the FIX/cmplx_removal branch October 2, 2024 21:11
@egthomas
Copy link
Member

egthomas commented Oct 4, 2024

Just to clarify, were the output of FITACF 2.5 or the map_addmodel / solve_model routines tested to make sure there was no change in behavior?

@RemingtonRohel
Copy link
Author

I did not test before it was merged, I've got no good excuse. I just verified that fitacf2.5 yields identical results, but map_addmodel -old_aacgm was giving different results for model.kvect and model.vel.median fields. It's a -1 error in in cnvmodel.c, I've made a change and verified that I get identical results. I've opened a new PR #623 with the change.

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