Skip to content

Fir shot on adaptor#5

Open
saxjax wants to merge 5 commits into
masterfrom
firShotOnAdaptor
Open

Fir shot on adaptor#5
saxjax wants to merge 5 commits into
masterfrom
firShotOnAdaptor

Conversation

@saxjax

@saxjax saxjax commented Oct 7, 2021

Copy link
Copy Markdown
Collaborator

No description provided.

@saxjax

saxjax commented Oct 7, 2021

Copy link
Copy Markdown
Collaborator Author

image

all tests are passed, I refactored a lot, and used the pytest -vv to spot errors

@saxjax saxjax requested a review from MartinBruun October 7, 2021 22:56

@MartinBruun MartinBruun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Koden er fuldt ud accepteret, men der er lige noget i testene jeg synes virker lidt mærkeligt.

Comment thread src/__pytest__/conftest.py Outdated
"highway": "residential",
"maxspeed": 80,
"edge_adj": 2,
"edge_adj": 3,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Denne forstår jeg ikke?
1 -> 3 gør jo at det ikke længere er en "straight line" som denne fixture i dens beskrivelse siger den er?

Ie.

Node 0 -> Node 1 -> Node 2 -> Node 3
(og modsat vej, givende 3*2 edges)

Hvis du mener at du prøver at modellere noget andet, så tænker jeg at beskrivelsen af denne "fixture collection" bør opdateres til hvad du mener det repræsentere :)

Comment thread src/__pytest__/conftest.py Outdated
"edge_adj": 6,
},
{
"edge_id": 6,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hvorfor er der behov for en 7. edge?
Tænker det vil være bedre at lave en ny "fixture collection" som modellere et mere sammenhængende netværk (som ser ud til at være det du ønsker?) Men at vi stadig skal have et "bare bones" / simpelt "straight line" netværk?

{
"to_node_id": 0,
"weight": 1,
"edge_id": 0,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Fjern edge_id, medmindre der er et rigtigt godt argument for at beholde det.

Comment thread src/__pytest__/conftest.py Outdated
"to_node_id": 1,
"weight": 1,
"edge_id": 3,
"to_node_id": 4,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Denne giver ikke mening, da vi jo kun har node 0, 1, 2 og 3.

Comment thread src/__pytest__/conftest.py Outdated
"to_node_id": 3,
"weight": 1,
"edge_id": 4,
"to_node_id": 5,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Denne giver ikke mening, da vi jo kun har node 0, 1, 2 og 3.

Comment thread src/__pytest__/conftest.py Outdated
"to_node_id": 2,
"weight": 1,
"edge_id": 5,
"to_node_id": 6,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Denne giver ikke mening, da vi jo kun har node 0, 1, 2 og 3.

Comment thread src/adapter.py
#nodesJSON = table_kwargs["nodes"]
# dataModel.errorList = table_kwargs["errors"]
# dataModel.metadata = table_kwargs["metadata"]
# dataModel.vehicle = table_kwargs["vehicle"]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Fjern kommentarene ;)

Comment thread src/adapter.py
for row in table_kwargs["nodes"]:
node=makeNode(row)
dataModel.nodes.insert(node.node_id, node)#add node at its node_id
dataModel["nodes"].insert(node["node_id"], node)#add node at its node_id

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Som udgangspunkt er jeg nok lettere uenig i at sætte kode kommentarer for hvad koden gør.

Tror grunden til vi gør det nu, er fordi python jo er et nyt sprog, så vi kan godt lade det stå, hvis det er.

@jchri17 jchri17 self-requested a review October 12, 2021 07:52
@saxjax

saxjax commented Apr 2, 2022

Copy link
Copy Markdown
Collaborator Author

Finno

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.

2 participants