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

(WIP) A couple of fixes for un-verified options. #33

Closed
wants to merge 5 commits into from

Conversation

michael-petersen
Copy link
Member

@michael-petersen michael-petersen commented Aug 9, 2023

This is a work-in-progress check of all options for CBrockDisk.

Some functionality was not usable previously, and I appear to have inadvertently introduced a memory problem in a previous commit.

This commit fixes these known bugs; more are likely present. Therefore, I propose a small to-do list (not strictly needed before merge, but maybe helpful):

  • Develop a test suite and commit to EXP-examples (maybe in a subdirectory for specialised tests?)
  • Test NO_M0, NO_M1, EVEN_M, M0_ONLY options to verify expected potentials are returned.

@The9Cat
Copy link
Contributor

The9Cat commented Aug 9, 2023

These changes look right to me. And, yes, great task list!

@michael-petersen michael-petersen self-assigned this Aug 9, 2023
@michael-petersen
Copy link
Member Author

michael-petersen commented Aug 19, 2023

Verified that all switches are working as expected; still mulling over test case. Also adding another todo:

  • Verify that symmetric norm is working as expected on test case

@michael-petersen
Copy link
Member Author

I'm thinking this can be merged in too: the test suite is effectively available from zangics now; it could be verified that the model runs as expected, but not sure that is crucial at this point.

The changes should still be brought in, since they fix some known issues with cbrockdisk itself.

@michael-petersen
Copy link
Member Author

As per our discussion regarding specialised basis sets, I think cbrockdisk may also be a candidate for non-inclusion in release, although it occupies a slightly unique space since it is properly 2d. But isn't that still strictly superseded by flatdisk?

@The9Cat
Copy link
Contributor

The9Cat commented Apr 3, 2024

Yes, flatdisk includes CluttonBrock 2d in a sense. Currently, all flatdisk bases are constructed by EOF using the base basis of either Bessel (bess) or Clutton-Brock (cb). The first is intended for disks with finite domain, and the second for disks with an infinite domain.

At this point, the target disk densities are exponential, Kuzmin, Mestel, and Zang. I intended to have a tabled basis as input (and that would not take much additional work) but haven't done that yet.

So, in principle, Clutton-Brock disk is there but not in a raw form.

Also, the 3d potential--the vertical force for halo coupling e.g.--works but the numerics make a less accurate solution for cb than bess; bess is remarkably accurate but one needs a truncated or finite model for that to make sense. I learned this while testing after implementation. So I sort of gave up on cb.

@michael-petersen
Copy link
Member Author

cbrockdisk has been removed from EXP; closing this issue.

@michael-petersen michael-petersen deleted the CBrockCheck branch May 29, 2024 15:37
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