-
Notifications
You must be signed in to change notification settings - Fork 6
Mock API for retrieving data from simulation run #96
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
base: main
Are you sure you want to change the base?
Conversation
src/components/SimulationVuer.vue
Outdated
| return response | ||
| } | ||
| defineExpose({ getDataView }) |
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.
Why getDataView? Why not simply getData?
| </template> | ||
|
|
||
| <script setup> | ||
| const emit = defineEmits(['data-ready']) |
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.
I would use data-available (or dataAvailable).
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.
In fact, this emit is not used, so why define it?
| if (type === 'sine') { | ||
| // Smooth curve. | ||
| data.push(Math.sin(i * 0.1) * 10) | ||
| } else { | ||
| // Random noise. | ||
| data.push(Math.random() * 10) | ||
| } |
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.
Might want to add some mock data for the variable of integration?
| // Check request is expected type. | ||
| if (req.id !== EXPECTED_ID) { | ||
| console.warn(`[Mock] Request #${index} ignored: Invalid ID '${req.id}'`) | ||
| return // Skip this specific item. | ||
| } | ||
| // Test version compatibility. | ||
| const requestMajorVersion = parseInt(req.version.split('.')[0]) | ||
| if (requestMajorVersion !== EXPECTED_MAJOR_VERSION) { | ||
| console.warn(`[Mock] Request #${index} ignored: Version mismatch. Expected v${EXPECTED_MAJOR_VERSION}.x, got v${req.version}`) | ||
| return // Skip this specific item. | ||
| } |
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.
I wouldn't bother with this, but... meh.
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.
Still wouldn't bother with this kind of check.
| if (req.variable && req.variable.includes('v_in')) { | ||
| response[req.identifier] = generateMockSeries('sine') | ||
| } else { | ||
| // Default fallback for other variables. |
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.
Also come here if req.variable is not truthy.
| } else { | ||
| // Default fallback for other variables. | ||
| response[req.identifier] = generateMockSeries('random') | ||
| } |
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.
I would expect req.variable to be something like membrane/V or sodium_current/i_Na. As for the variable of integration, we could use VOI as a special variable name. So, here, I would expect to add a case for where req.variable === 'VOI'?
agarny
left a comment
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.
It would be nice to have a PR that doesn't consist of 99% of reformatted code. It makes the PR unnecessarily difficult to review.
This aside, I understand that you expose getData():
- I would probably called that method
getSimulationData(). - It's not clear to me what the
requestsparameter is supposed to look like.
| if (!requests || !Array.isArray(requests)) { | ||
| console.error("getDataView: Request must be an array.") | ||
| return {}; | ||
| console.error('getData: Request must be an array.') |
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.
"request", not "Request".
| function generateMockSeries(type, length = 100) { | ||
| const data = [] | ||
| for (let i = 0; i < length; i++) { |
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.
Very minor since this is bound to be removed, but: ++i rather than i++.
| // Check request is expected type. | ||
| if (req.id !== EXPECTED_ID) { | ||
| console.warn(`[Mock] Request #${index} ignored: Invalid ID '${req.id}'`) | ||
| return // Skip this specific item. | ||
| } | ||
| // Test version compatibility. | ||
| const requestMajorVersion = parseInt(req.version.split('.')[0]) | ||
| if (requestMajorVersion !== EXPECTED_MAJOR_VERSION) { | ||
| console.warn(`[Mock] Request #${index} ignored: Version mismatch. Expected v${EXPECTED_MAJOR_VERSION}.x, got v${req.version}`) | ||
| return // Skip this specific item. | ||
| } |
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.
Still wouldn't bother with this kind of check.
| </template> | ||
|
|
||
| <script setup> | ||
| const emit = defineEmits(['data-ready']) |
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.
In fact, this emit is not used, so why define it?
| requests.forEach((req, index) => { | ||
| // --- VALIDATION BLOCK --- | ||
| // Check request is expected type. |
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.
// Check that the request is of the expected type.
| const EXPECTED_ID = 'nz.ac.auckland.simulation-data-request' | ||
| const EXPECTED_MAJOR_VERSION = 0 | ||
| requests.forEach((req, index) => { |
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.
It's not 100% clear to me what requests is expected to look like.
| // Check request is expected type. | ||
| if (req.id !== EXPECTED_ID) { | ||
| console.warn(`[Mock] Request #${index} ignored: Invalid ID '${req.id}'`) |
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.
"invalid", not "Invalid". Also need a final stop.
| if (req.position) { | ||
| response.position = { ...req.position } | ||
| } |
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.
What is the point of req.position? It is in the request and then put in the response without doing anything with it in between?
| response.position = { ...req.position } | ||
| } | ||
| if (!response['data']) { |
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.
Isn't this test always true since we start with response = {}?
No description provided.