[NAT-4] NAT Rooms #495

octycs · 2023-04-01T16:14:17Z

Resolves #34

Proposed Changes (include Screenshots if possible)

Merge rooms and their additional data from the NAT roomfinder

How to test this PR

Recompile data
Check in the UI what is included

How has this been tested?

Verified that the additional data is visible on the website

Checklist:

I have updated the documentation / No need to update the documentation
I have run the linter

…ep the names defined in the areatree; I verified by hand that the names from the NAT roomfinder do not contain more information than what we have already. They are mostly from the MyTUM Roomfinder)

…ree so far

octycs · 2023-04-02T15:41:13Z

This is finished mostly. Only three things are missing currently:

Seating plans (example) that are linked in the NAT roomfinder are not given in the API
Some properties (usually comments) are currently not exported. I'm not sure how to best handle properties such as bauarbeiten or corona because they have a very mixed usage and might be outdated
Coordinates are not merged at the moment. The is because I haven't assessed their precision yet

CommanderStorm

These are some preliminary things I have found.

I have to look into the code a bit deeper, as there are some things (some code paths+how it looks in the frontend) I would like to investigate

data/processors/nat.py

CommanderStorm · 2023-04-02T16:23:26Z

data/processors/nat.py

+    )
+
+
+def _is_room_excluded(internal_id, b_id, data):


Could you add a Docstring (imo this function does a bit of magic which I don't fully understand why yet)? 😅

CommanderStorm · 2023-04-02T16:24:10Z

data/processors/nat.py

+    return True
+
+
+def _get_room_base(internal_id, b_id, nat_data, data, usages):


Could you add a Docstring (you can probably betterdescribe what this function does)? 😅

CommanderStorm · 2023-04-02T16:25:48Z

data/processors/nat.py

+    if len(building_sources) == 2 and building_sources[1]["name"] == "NAT Roomfinder":
+        return False
+
+    return True


Suggested change

if len(building_sources) == 2 and building_sources[1]["name"] == "NAT Roomfinder":

return False

return True

return len(building_sources) != 2 or building_sources[1]["name"] != "NAT Roomfinder"

CommanderStorm · 2023-04-02T16:36:42Z

data/processors/nat.py

+
+
+def _merge_nat_extra_props(r_data, nat_data):
+    # pylint: disable=too-many-branches


These are many branches, can we extract something into its own method?

I think infrastructure (seating, streaming, sockets) would be fitting

data/sources/00_areatree

CommanderStorm

About the frontend-changes (here and not inline commented, to keep things more dense):

I would prefer alternative Seating Configurations Seating Plan
I would the seating to be displayed as a list. given that this is an eazy fix we can also postpone this one into another PR if you'd like
I would prefer Straming avalible via ... as a link text for hörsaal.webcam and linking to tum.live/about instead of the wiki entry for the RBG

What do you think?

octycs · 2023-04-04T09:59:39Z

I would the seating to be displayed as a list. given that this is an eazy fix we can also postpone this one into another PR if you'd like

I used a comma because it seems a list needs a change in the frontend (I tried to at least use \n instead of , first, but it didn't work).

I would prefer Straming avalible via ... as a link text for hörsaal.webcam and linking to tum.live/about instead of the wiki entry for the RBG

Given that only a handful of rooms have this property, should we rather add the data outselves in 02_rooms-extended.yaml. This way the data is more at a central place in case anything changes in the future (Did I understand it correctly, that the NAT roomfinder is to be discontinued?)

Co-authored-by: Frank Elsinga <frank@elsinga.de>

data/processors/nat.py

CommanderStorm · 2024-07-20T22:08:06Z

I am going to close this due to the high amount of merge conflict.

I don't think navigatum should show the extended seating information as TUM has bought the TUMexam service pack and this is one of the things they do.
=> I don't think we should offer the seating information any more

The other information (e.g. extra rooms) has somewhat dubeous data quality and is thus quite hard to merge.. I don't have a good solution on this one.

CommanderStorm and others added 9 commits April 1, 2023 10:38

implemented a basic way to merge nat rooms

8dd0ae2

Improve ID handling for buildings and skip name merge (This way we ke…

edc7051

…ep the names defined in the areatree; I verified by hand that the names from the NAT roomfinder do not contain more information than what we have already. They are mostly from the MyTUM Roomfinder)

Update latest NAT scrape data

12e29d8

Research current status and coordinates of buildings not in our areat…

bef4cd2

…ree so far

Fix Feuerwehr Garching according to OSM

dabc257

Fix export

206f1d6

Run linter

2261efd

Update data format doc

f882e33

Determine which rooms to merge

5762f16

octycs added the data This issue is related to the data collection and aggregation label Apr 1, 2023

octycs self-assigned this Apr 1, 2023

octycs marked this pull request as draft April 1, 2023 16:15

octycs mentioned this pull request Apr 1, 2023

[NAT-3] Nat Buildings #362

Merged

5 tasks

Base automatically changed from nat-merger to main April 1, 2023 20:33

octycs added 3 commits April 1, 2023 22:48

Merge branch 'main' into nat-room-merge

ec63425

Merge room base data (without coords)

dfa44bd

Add extra information from the NAT roomfinder

16cdf10

octycs marked this pull request as ready for review April 2, 2023 15:36

octycs requested a review from CommanderStorm April 2, 2023 15:41

CommanderStorm reviewed Apr 2, 2023

View reviewed changes

CommanderStorm requested changes Apr 2, 2023

View reviewed changes

CommanderStorm mentioned this pull request Apr 3, 2023

[NAT-3.9] Less insane NAT scraper #497

Merged

2 tasks

CommanderStorm and others added 3 commits April 5, 2023 15:10

Merge branch 'main' into nat-room-merge

a105588

Apply suggestions from code review (minor changes)

4c0d801

Co-authored-by: Frank Elsinga <frank@elsinga.de>

Add docstrings, simplify if, fix variable names

45e1bea

CommanderStorm reviewed Apr 10, 2023

View reviewed changes

data/processors/nat.py Outdated Show resolved Hide resolved

CommanderStorm marked this pull request as draft June 4, 2023 18:52

Update data/processors/nat.py

f31cda2

Update data/processors/nat.py

792dc88

CommanderStorm closed this Jul 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NAT-4] NAT Rooms #495

[NAT-4] NAT Rooms #495

octycs commented Apr 1, 2023 •

edited by CommanderStorm

Loading

octycs commented Apr 2, 2023

CommanderStorm left a comment

CommanderStorm Apr 2, 2023

CommanderStorm Apr 2, 2023

CommanderStorm Apr 2, 2023

CommanderStorm Apr 2, 2023

CommanderStorm left a comment

octycs commented Apr 4, 2023

CommanderStorm commented Jul 20, 2024

		return True


		def _get_room_base(internal_id, b_id, nat_data, data, usages):



		def _merge_nat_extra_props(r_data, nat_data):
		# pylint: disable=too-many-branches

[NAT-4] NAT Rooms #495

[NAT-4] NAT Rooms #495

Conversation

octycs commented Apr 1, 2023 • edited by CommanderStorm Loading

Proposed Changes (include Screenshots if possible)

How to test this PR

How has this been tested?

Checklist:

octycs commented Apr 2, 2023

CommanderStorm left a comment

Choose a reason for hiding this comment

CommanderStorm Apr 2, 2023

Choose a reason for hiding this comment

CommanderStorm Apr 2, 2023

Choose a reason for hiding this comment

CommanderStorm Apr 2, 2023

Choose a reason for hiding this comment

CommanderStorm Apr 2, 2023

Choose a reason for hiding this comment

CommanderStorm left a comment

Choose a reason for hiding this comment

octycs commented Apr 4, 2023

CommanderStorm commented Jul 20, 2024

octycs commented Apr 1, 2023 •

edited by CommanderStorm

Loading