1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

LSUThorns/Vectors: Remove pos, add sin/cos/tan functions

Issue #683 closed
Erik Schnetter created an issue

Remove kpos, because it is not used (it is a no-op, i.e. the arithmetic + operator).

Add sin, cos, and tan.

Add comments describing how integer vector operations could be added.

Keyword:

Comments (10)

  3. Barry Wardell
    • removed comment

    The kpos and trig changes look fine to me, although they could be logically split into separate patches.

    Why are the integer vector operations only added as comments?

  4. Erik Schnetter reporter
    • removed comment

    They are only added as comments because they are incomplete. I began to implement this, and then decided to go another route. The implementation is correct, but incomplete. What is missing is mostly the definition or a corresponding integer vector type.

  5. Barry Wardell
    • removed comment

    In that case, I would suggest committing the trig and pos changes now and then creating a new ticket for the integer bits once they are ready. If you're trying a different approach anyway, I can't imagine the commented out implementation is of much benefit right now.

  6. Erik Schnetter reporter
    • removed comment

    You are asking me to keep uncommitted changes in my repository. This is very inconvenient for me, because that means I have to use diff and patch (and emacs) to create patches, and a different source tree for testing. Also, if someone else wants to work on vectorising integer operations, they'll have to begin from scratch since they don't know about what I did, and if this person implements something from scratch, it will likely be different from mine, making my work irrelevant.

    I think we're going way too far here with keeping the trunk and its history clean and perfect. Imagine when EinsteinExact and Metrics will be in the ET -- do you really want to be kept to the same standards when you continue to develop them, with people asking you to break up your existing commits, and asking you to keep commits to yourself unless they are needed, at which time they will be reviewed (and probably changed, requiring you to change other code that relies on them)? It's good to keep code clean, but that has to happen at a much higher level. We should e.g. be discussing the vectorisation API, or which archtictures we want to test regularly, or how we can test performance (which is necessary since vectorisation is a performance optimisation). If we discuss whether to keep a few lines of commented-out code, we've reached a far too low level, we're looking at twigs instead of the forest.

  7. Frank Löffler
    • removed comment

    I agree with both of you. Yes, changes which are not ready should not be committed. In most cases anyway. In this case you made a good point of why they should anyway Erik. What do you think about protecting the integer code with a single #define, which by default disables the code, and which has a comment attached explaining that this is not yet fully implemented? This way you would not have uncommitted code, and users would know why this code isn't used yet.

  8. Barry Wardell
    • removed comment

    I agree that this is not worth arguing over. If you really find it makes your life easier, then I won't complain about committing the commented out lines. I was just under the impression that you had a better and different system in the works which meant that committing these lines wasn't going to be much help anyway.

  11. Log in to comment
Assignee
–
Type
enhancement
Priority
minor
Status
closed
Component
EinsteinToolkit thorn
Milestone
–
Version
–
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!