Skip to content
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

Test update, ruff checks, deprecations warnings and import checks. #190

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

GinaStavropoulou
Copy link

In this PR:

TESTS:

  • I have updated the tests: some were removed as they were obsolete, some were updated.
  • Only a single test is failing (see below). The error is related cjval and @hugoledoux will fix it.
image
  • No new test was added for now but I did check the coverage, here are the current results:
image

IMPORTS:

  • Since various imports depend on which installation we are running I added import checks in the init file. This way we can remove the checks in every file.

RUFF CHECKS & FORMAT:

  • I run the ruff check and I corrected all the errors I was getting there (including substituting a lambda function)
  • I also used the formatter and it seems that some files (eg model.py, convert.py) had not been formatted before. Also the tests have not been formatted yet.

I will leave some extra comments below in some things that need attention



loader = importlib.util.find_spec("triangle")
Copy link
Author

Choose a reason for hiding this comment

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

The imports are all checked in this file and in the test of the software the consts are imported from here.

@@ -517,7 +500,7 @@ def validate(self):
# print("Downloading the Extension JSON schema file(s):")
if "extensions" in self.j:
for ext in self.j["extensions"]:
theurl = self.j["extensions"][ext]["url"]
Copy link
Author

Choose a reason for hiding this comment

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

I disabled this line because it was not used.

@@ -1054,7 +1038,10 @@ def get_info(self, long=False):
if geom["semantics"] is not None:
for srf in geom["semantics"]["surfaces"]:
sem_srf.add(srf["type"])
getsorted = lambda a: sorted(list(a))
Copy link
Author

Choose a reason for hiding this comment

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

This change was required by ruff

@@ -1430,27 +1415,7 @@ def merge(self, lsCMs):
raise KeyError(
f"The member 'values' is missing from the texture '{m}' in CityObject {theid}"
)
# -- metadata
if self.has_metadata_extended() or cm.has_metadata_extended():
Copy link
Author

Choose a reason for hiding this comment

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

this code had to be removed as there are no metadata_extended anymore

@@ -2047,17 +2011,6 @@ def filter_lod(self, thelod):
self.remove_duplicate_vertices()
self.remove_orphan_vertices()
self.update_bbox()
# -- metadata
if self.has_metadata_extended():
Copy link
Author

Choose a reason for hiding this comment

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

as above, this too had to be removed.

nodes.append({
"matrix": [1, 0, 0, 0, 0, 0, -1, 0, 0, 1, 0, 0, 0, 0, 0, 1],
"children": [],
"name": cm.j.get("metadata", "citymodel").get("identifier", "citymodel")
Copy link
Author

Choose a reason for hiding this comment

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

this was failing if there were no 'metadata' so I changed it. @hugoledoux can you verify that it is ok?

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea, never worked wiht glTF. I didn't write that code...

but cityjson/metadata is not mandatory, so yes it could crash...

but deleting seems wrong (@balazsdukai you know more I reckon?) since it caused a missing matrix. Wouldn't it be better to just put something else for "name"?

Copy link
Member

Choose a reason for hiding this comment

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

The "name" is not required, so it is safe to omit it.

print(i, v)
vtx_idx_np[i] = i
bin_vtx = vtx_np.astype(np.float32).tostring()
Copy link
Author

Choose a reason for hiding this comment

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

deprecation warning.

@@ -11,7 +11,7 @@ license_file = LICENSE

[tool:pytest]
log_cli = true
collect_ignore = ['setup.py']
Copy link
Author

Choose a reason for hiding this comment

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

deprecation warning

],
'export': [
'pandas',
'mapbox-earcut',
'triangle'
'triangle2'
Copy link
Member

Choose a reason for hiding this comment

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

@GinaStavropoulou do we want triangle2 though? This is macOS specific, others have triangle

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.

3 participants