Skip to content

Conversation

@PaulWessel
Copy link
Member

Currently, the modifiers +m and +M are mutually exclusive and thus it is only possible to output intermediary grids for the cumulative or incremental solution. However, since the main calculation time is in determining the solution, it is helpful to be able to write both types of grids at the same time. This PR replaces the +m and +M with two new modifiers that both can be set: +c for cumulative grids and +i for incremental grids. We now use the regular -Ggrdfile setting and automatically insert _inc_### or _cum_### before the file extension (the number of ### is also determined), and we start numbering with eigenvalue 0 rather than calling it 1 (everything else in GMT starts at 0 so starting at 1 causes grief in batch and movie). The old +m|M modifiers are honored in a backwards compatible way with the original file name template and numbering scheme. All tests pass and a new replacement anim05.sh using the new system works as well.

Currently, the modifiers +m and +M are mutually exclusive and thus it is only possible to output intermediary grids for the cumulative or incremental solution.  However, since the main calculation time is in determining the solution, it is helpful to be able to write both types of grids at the same time.  This PR relaces the +m and +M with two new modifiers that both can be set: +c for cumulative grids and +i for incremental grids.  We now use the regular -Ggrdfile setting and automaticallly insert _inc_### or _cum_### before the file extension, and we start with eigenvalue 0 rather than calling it 1 (averything else in GMT starts at 0 so starting at 1 causes grief in batch and movie). The old +m|M modifier is honored in a backwards compatiple way with the original file template and mumbering scheme.
@PaulWessel PaulWessel added enhancement Improving an existing feature deprecation Deprecating a feature labels Aug 30, 2021
@PaulWessel PaulWessel self-assigned this Aug 30, 2021
@maxrjones
Copy link
Member

The example from the greenspline docs crashes with a seg fault:
gmt greenspline @Table_5_11.txt -R0/6.5/-0.2/6.5 -I0.1 -Gcontribution.nc -Sc -Z1 -C+c

@PaulWessel
Copy link
Member Author

Yes it does! Of course I did not try that simple example, just my more complicate one. Will see why.

@PaulWessel
Copy link
Member Author

Fixed. My case worked because I used +i also. Now it works in all cases.

Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Two comments:

  1. The results from -C +ffile are written with 1 as the first eigenvalue. Should this be updated to zero similar to the grids?
  2. From the examples, value is optional. The docs should be updated accordingly with the default behavior specified.

Also, the -C[n] has an impact on which eigenvalues are eliminated for the fit, but not for what is written in the +f file output or the intermediate grids. Is this intentional?

@PaulWessel
Copy link
Member Author

Good points, likely answers:

  1. We should be consistent using the 0-based counting, so first eigenvalue should be number 0.
  2. I will update the docs regarding the optional value
  3. Unintentional, and I think if someone gives a value for n then they probably do now want to run this all the way but stop at n. I will make it that way.

@PaulWessel
Copy link
Member Author

Yes, I forgot to make a PS and check for height. I will do that and possibly add a -Y or change the plot heights a bit. Thanks!

@PaulWessel
Copy link
Member Author

I have made those changes and clarified some of the logic which is due to the fact that the eigenvalues/vectors we receive are not sorted so we set those we dont want to zero, but those zeros are distributed across the full vector, hence we must loop over the length of the eigen value vector and skip if entry is zero.

@PaulWessel
Copy link
Member Author

Also fixed the compile failure for Windows due to unsigned into loop variable in OpenMP...

@PaulWessel PaulWessel merged commit a032682 into master Aug 31, 2021
@PaulWessel PaulWessel deleted the greenspline-movie branch August 31, 2021 22:32
@maxrjones maxrjones added the add-changelog Add PR to the changelog label Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add-changelog Add PR to the changelog deprecation Deprecating a feature enhancement Improving an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants