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

potential way of implementing discussion #587 #592

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Conversation

mreineck
Copy link
Collaborator

This demonstrates how variables depending on problem dimensionality could be merged into std::arrays, and how it potentially simplifies the code.

This is not yet complete:

  • I'm not happy with the variable names yet. For example, I renamed nf1, nf2, nf3 to nf123, since the obvious candidate nf was already used for storing the product nf1*nf2*nf3, and I didn't want to do big refactorings for this demonstrations.
  • there are more good candidates in the interface of spreadinterp, which I haven't looked at much, mostly because there is a clash between finufft_core.h, where integer quantities are usually BIGINT,and spreadinterp.h, where they tend to be UBIGINT. While it is possible to convert individual integers on the fly when crossing the interface, the same does not work for std::arrays, so I tought I'd ask for opinions before taking this any further.

@ahbarnett
Copy link
Collaborator

THanks Martin, THis is a good idea, in the category "incremental simplifications that do not affect performance", right?
Personally, since index arithmetic can product <0 integers, I would switch everything to BIGINT in spreadinterp, if this allows. I'm sure we could bring this in, esp, if it's needed for other simplifications.

@DiamonDinoia
Copy link
Collaborator

I welcome this change. Next year, when c++26 will come out, I aim to use c++20 & spans to further clarify this.

Any reason for the draft tag?

@mreineck
Copy link
Collaborator Author

I only added the draft tag because of the clumsy variable names. If you can live with them until better ones are found, I think it can be merged.

@DiamonDinoia
Copy link
Collaborator

On the naming, I think @ahbarnett is the best person to ask

@mreineck
Copy link
Collaborator Author

mreineck commented Jan 1, 2025

The failing Matlab tests look like some unreachable servers (related to the tmate action, whatever that is).

@ahbarnett ahbarnett self-assigned this Jan 6, 2025
@ahbarnett
Copy link
Collaborator

I'm bringing this in, and fix up later. I will rename nf123 as nfdim; others are acceptable.

PS I have never liked the push to UBIGINT, and now I know why: http://soundsoftware.ac.uk/c-pitfall-unsigned.html I would like to use signed ints throughout, as we used to - thoughts?

Copy link
Collaborator

@ahbarnett ahbarnett left a comment

Choose a reason for hiding this comment

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

I may tweak some variable names. Clearly deconvolveshuffle{1,2,3}d would be better as a class method, etc. This tidying can happen slowly. There are more important performance tweaks to do...

@ahbarnett ahbarnett marked this pull request as ready for review January 7, 2025 03:03
@ahbarnett ahbarnett merged commit 4e29368 into master Jan 7, 2025
331 of 332 checks passed
@mreineck
Copy link
Collaborator Author

mreineck commented Jan 7, 2025

PS I have never liked the push to UBIGINT, and now I know why: http://soundsoftware.ac.uk/c-pitfall-unsigned.html I would like to use signed ints throughout, as we used to - thoughts?

I'm fine with signed ints as well, though I disagree with the article's author that the error potential is much higher when using exclusively unsigned types - I'd say it balances out more or less.

Counterargument 1 is the strongest, as the article states. Unfortunately no workaround/mitigation is proposed - that means we need a check whenever we access a vector size, strictly speaking.

“Unsigned overflow and arithmetic is better-defined” no longer holds in very recent versions of C/C++; overflow of signed intergers now has well-defined semantics, so we needn't wory about this one at least.

"Avoid using unsigned ints in C and C++ unless you're dealing with raw memory contents" ... this guideline is not as specific as it looks, because we are dealing with raw memory contents every time we put one of our integers in square brackets, and we do that with almost all of them :-/

@mreineck
Copy link
Collaborator Author

mreineck commented Jan 7, 2025

For completeness' sake, I'd like to add one more comment on the example from http://soundsoftware.ac.uk/c-pitfall-unsigned.html:

    for (unsigned i = 0; i < nlengths; i++) {
        unsigned candidates = total - lengths[i] + 1;
        for (unsigned j = 0; j < candidates; j++) {
            // use j as an array index

This is written in a way that makes it look like the problematic subtraction is unavoidable. The "right" way of writing this loop would be

    for (unsigned i = 0; i < nlengths; i++) {
        for (unsigned j = 0; j+lengths[i] <= total; j++) {
            // use j as an array index

which does not break and (at least to me) also expresses more concisely what is going on.

The problematic subtractions can be avoided in practically all cases, but I agree this is a gradual learning process (I was a "signed-integer only" person myself for a long time, and the transition took me about a year). Either choice of signedness requires careful checking in unexpected places, and even with hindsight I couldn't say if any alternative is objectively better.

@ahbarnett ahbarnett deleted the discussion_587 branch January 8, 2025 23:22
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.

3 participants