-
Notifications
You must be signed in to change notification settings - Fork 1
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
Merging the initial High Level requirements #2
Conversation
Adding the initial high level requirements in a separate branch and creating a pull request to be merged into the Math_library branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to the changes
This has been completed but we need someone to review it |
This PR is associated to #1 |
High_Level_Requirements
Outdated
@@ -0,0 +1,5 @@ | |||
High Level requirements for the math library | |||
Math features | |||
Sine - This featute shall take an input value and calculate and return the sine value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature is mis-spelled. Also we should add some context that allows people to see if there are LLRs associated with it. Right now, i would say that there needs to be some range bounds defined somewhere (usually in an LLR), but I dont know from looking at this screen if there are LLRs defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Will update spelling.
From this screen we will only see changes between branches. You could always look a the file by pressing the button "View file"
In the LLRs I did state that this function shall take in degree's and radians but that was un-bounded. Should bounds be defined at the HLR level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bounds would normally be defined in the LLRs.... its hard to see the full trace from here, and i didnt see the trace in the view file view.... i'm sure you showed me this before but i'm not seeing it now....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where to view the file. This is could be helpful if you wanted to see the whole file and if this only showed the Diffs
- I will add comments for the links.
This isnt exactly how doxygen requirements would be represented. Ideally we would have a links related to this requirement where there was a reference to the LLR's. You would have to manually trace it within the repo or I would envision people would go to the rendered Doxygen to be able to do the tracing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added links but I also had to add unique id's in the LLRs. That change was based on this commit 520fa77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sine - This featute shall take an input value and calculate and return the sine value | |
Sine - This feature shall take an input value and calculate and return the sine value |
High_Level_Requirements
Outdated
High Level requirements for the math library | ||
Math features | ||
Sine - This featute shall take an input value and calculate and return the sine value | ||
Cosine - This featute shall take an input value and calculate and return the sine value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature mis-spelled, also should return cosine value, not sine value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Will update spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- And update requirement to remove the correct value
Updating LLR links based on this commit: 520fa77
Adding the initial high level requirements in a separate branch and creating a pull request to be merged into the Math_library branch.