Preflight checks hackathon 2024

[jmcouffin]

Hi guys,
@dosymep @sanzoghenzo @eirannejad @GiuseppeDotto @thumDer
@dnenov @davidvadkerti @dariaivanciucova
Would you give care to give me your evaluation of the work done by the few preflight checks hackathon participants?
It should not take you long, I hope to collect your input to provide the results by next monday.

• cad_audit
• walltypes_naming_convention
• and radar_check

set the order and add comments following my template to evaluate:

1. radar_check
   I like the idea, it is pretty elaborate
2. cad_audit
   fair entry, coding style is a bit weak
3. walltypes
   the idea is good, it lack a bit of love, and could be extended to lots of naming_convention checks on the same principes adding a UI/UX to configure the check

[sanzoghenzo]

• uses a json file just for a list of strings, it might be just a plain text file

I think here, json is more appropriate for future scalability Making this check, checking different categories for naming convention

Ok, but the current function doesn't allow to do it, it just returns a list of names.
It should return the entire dictionary loaded from the json (then it could even be moved to a pick_json function in the pyrevit library, returning None if the file is not chosen) and the access to the allowed_wall_types values should be done in the main function.

[sanzoghenzo]

I still have to tackle the radar check, for that I need quite some time 😅

[sanzoghenzo]

else not needed since the if branch returns

[tay0thman]

• some functions are too long and returns many variables; it is more or less trivial to split them into smaller, simpler functions
• some functions are called twice or more with the same parameters, the result should be stored in a variable and reused
• other un-pythonic things here and there, formatting needs some fixes

no time for this @tay0thman maybe you want to take the time to do this?

@jmcouffin , I'll go and start cleaning this myself, likely this weekend.

[sanzoghenzo]

I'm halfway through the review of the radar check, but the cognitive load of the main function is too high for me to discover other things to refactor.

I'll do a second round once the function is split into more manageable chunks

[sanzoghenzo]

remove

[sanzoghenzo]

make it a docstring for the function

[sanzoghenzo]

remove

[sanzoghenzo]

This is valid for all the other lines below: use the str.format() method instead of string concatenation

[sanzoghenzo]

repeated calculation, extract the horizontal distance into a variable (or maybe that function should be 3 functions that return a single value each!)

[jmcouffin]

@sanzoghenzo Thank you for all that, it is a learning experience for me and will force me to review my code more carefully before submitting to the repo.
❤️ ur code reviews
Keep them coming!

[tay0thman]

@sanzoghenzo , Thank you for your time! These are tips and lessons I could never learn from a book. I’ll go ahead and make these updates.

[dosymep]

radar_check
Idea: Good idea to display all coordinates to check that everything is nearby.

Interface: I think all real numbers could have been rounded to the third digit.

Code: The code is hard to read, the algorithm needs more decomposition into methods and classes. Lacks PEP8 compliance. It makes sense to create a check view once to speed up the check.

cad_audit
Idea: Showing users that some files were imported and duplicated sounds interesting.

Interface: All the properties of interest are displayed in the interface, everything looks good.

Code: The code generally looks good, lacks PEP8 compliance. Cool, I didn't know that you can create an interface (FlexForm) in pyRevit this way.

walltypes
Idea: A great idea for checking models and naming compliance. It would be cool to expand this check to other categories. Add the ability to do more complex checks: contains, starts with, ends with, exact match, regular expression matches, name matches with parameter value, and others.

Interface: I recommend making a settings template file, and when a user runs a check for the first time, a default check file would be created somewhere in my documents, which he would need to configure (I need to think about the logic).
After modifying the check, I will have to make a large and complex user interface with all the settings.

Code: There is not enough code, the check is too simple. I liked that the internal pyRevit framework is used.

---

On average, everyone has problems with the code, it is unnecessary complexity, there is practically no use of OOP, weak compliance with pep8, there is no interface using MVVM.

---

In general, thanks to everyone who participated.

[dosymep]

use LinkedFileStatus.NotFound and etc

extra conversion to a string worsens performance, creates an unnecessary object in memory, which will then have to be removed by the garbage collector, and also comparing a string is much slower than comparing an integer type.

[dosymep]

useless check, anyway if there is an empty list, it will not be enumerated.
The rules of good form say that it is better to return an empty list instead of None

[andreasdraxl]

Thank you for the response! There are many suggestions and insights here to build on. In my entire career, no one has really talked about my code — it was always just "as long as it works!" I can already get a lot out of this constructive "critique" and am starting to understand more and more what really matters in code architecture! @dosymep @jmcouffin 🚀🚀🚀

[sanzoghenzo]

There are still some thing that I missed, but again the cognitive load kicked in 😅

[sanzoghenzo]

Sorry if I wasn't clear before, but regions are usually used to group concepts/logical functionality, not python primitives.

there's no value to add a region for imports (most IDEs can collapse them automatically), constants (well, maybe this could help) and "functions" in general. Use regions, for example, for "calculate distance", "collect elements" and so on

[tay0thman]

I started using them on the large functions, thank you for the tip.

[sanzoghenzo]

Note that if you find yourself using regions in functions because they are large, probably they are too large, and regions could become separate, shorter and simpler functions 😉

[sanzoghenzo]

toggle here is a bad name, and we don't really need it because we can just check if class_type is something

[sanzoghenzo]

Suggested change

	if toggle:
	elements = (DB.FilteredElementCollector(document).
	WhereElementIsNotElementType().
	OfClass(class_type).
	ToElementIds())
	else:
	elements = (DB.FilteredElementCollector(document).
	WhereElementIsNotElementType.
	ToElementIds())
	return elements
	elements = (
	DB.FilteredElementCollector(document).
	WhereElementIsNotElementType()
	)
	if class_type:
	elements = elements.OfClass(class_type)
	return elements.ToElementIds()

I also see that you always call this function with a class_type, so the if branch only adds cpu time with no added value. I don't believe you'll ever need to fetch all the items of all the classes in a document, so just use

    return (
        DB.FilteredElementCollector(document)
        .OfClass(class_type)
        .WhereElementIsNotElementType()
        .ToElementIds()
    )

[sanzoghenzo]

If you need to search for one item matching the criteria, the next function with a generator saves CPU time and memory, since it exits as soon as a match is found instead of calculating all the elements beforehand.

Suggested change

	three_d_view_type = ([v for v in view_3D_types
	if v.ViewFamily == DB.ViewFamily.ThreeDimensional][0])
	three_d_view_type = next(
	v for v in view_3D_types if v.ViewFamily == DB.ViewFamily.ThreeDimensional
	)

[sanzoghenzo]

Suggested change

	view.IsolateElementTemporary(object.Id)
	view.IsolateElementTemporary(object.Id)

Also, you never pass an object to this function, you might as well remove these two lines and the argument from the furnction signature

[sanzoghenzo]

Suggested change

	if basptdistance > EXTENT_DISTANCE:
	if abs(basptdistance) > EXTENT_DISTANCE:

[sanzoghenzo]

This is a formatting nightmare, and could be simplified.
I personally prefer the black/ruff style formatting (if a list is too long, brackets are on their own lines and the should be one per line, indented from the brackets), but I understand that it is not the general code style inside pyrevit (because we never had one 😅 )

since the convert_unit and convert_value are always inside a str, why not move the string conversion inside those functions and drop the str here?
The second and third line are the same, it is just the point and the distance that differs; they can be turned into a function that returns the line (the angle can be calculated inside that very function)
There's still the doc / document variables problem.

[tay0thman]

does this function below makes sense? I am getting multiple returns:

def points_distances_angles(point):
    """
    Calculates the distances and angles between the internal origin and a point.
    Args:
        point (tuple): The coordinates of the point.
    Returns:
        distances (tuple): The distances between the internal origin and the point.
        angles (tuple): The angles between the internal origin and the point.
        x (string): The unit conversion value x-coordinate of the point.
        y (string): The unit conversion value y-coordinate of the point.
        z (string): The unit conversion value z-coordinate of the point.
    """
    x, y, z = point
    x = convert_values(x)
    y = convert_values(y)
    z = convert_values(z)
    distances = convert_units(abs(calculate_distance(point, INTERNAL_ORIGIN)))
    angles = convert_units(calculate_angle(point, INTERNAL_ORIGIN))
    return x, y, z, distances, angles

[sanzoghenzo]

The point I was making for multiple returns is not that it is forbidden, but it is useless to have multiple returns if you then use only one of the returned values, as it was done in the original script. Multiple returns are OK if they are used together by the calling function.
What I meant in this particular case, though, is this

def report_point(point, distance, document):
    angle = calculate_angle(point, INTERNAL_ORIGIN)
    return (convert_values(point[0], document), convert_values(point[1], document), convert_values(point[2], document), convert_units(distance, document), angle)

so you then can use

tbdata = [
    # ...
    ['Project Base Point Coordinates to Internal Origin', *report_point(baspt, basptdistance, document)],
    ['Survey Point Coordinates to Internal Origin', *report_point(survpt, surveydistance, document)],
    # ...
]

Another thing that can be done is to transform the convert_values to a function that accepts any number of values to convert and convert all the coordinates at once:

def convert_values(*values, document=doc):
    # ui_units logic unchanged...
    # ...
    return [
        "{:.3g}".format(DB.UnitUtils.ConvertFromInternalUnits(value, ui_units))
        for value in values
    ]

and then

def report_point(point, distance, document):
    angle = calculate_angle(point, INTERNAL_ORIGIN)
    return (*convert_values(*point, document=document), convert_units(distance, document), angle)

you could even parametrize the message and add it to the report_point function, so that you can just use

tbdata = [
    # ...
    report_point("Project Base Point ", baspt, basptdistance, document),
    report_point("Survey Point", survpt, surveydistance, document),
    # ...
]

[tay0thman]

this is crazy good!!
I have this table function to cleanup, I learned a lot of good tricks already.

[tay0thman]

this unpacking technique is very efficient, I should learn it more and capitalize on it. thank you for sharing.

[sanzoghenzo]

Don't use a global variable, just return 0 or 1 and and do the test_score calculations in the main function

[tay0thman]

I like this idea; I am returning an integer of 0 or 1 and add them in the start test function:

    def startTest(self, doc, output):
        with revit.DryTransaction("Checking Model Extents"):
            check_model_coordinates(doc)
            task_1 = check_model_extents(doc)
            task_2 = check_design_options(doc)
            if task_1 + task_2 < 2:
                output.print_md(
                    ('{} 3D View Bounding Box extends more than {}'.
                     format(BAD_STRG, CRITERIA_STRG)))
                check_linked_elements(doc)
                check_all_elements(doc)
            else:
                output.print_md(
                    ('{} 3D View Bounding Box is located less than {}'.
                     format(GOOD_STRG, CRITERIA_STRG)))

[sanzoghenzo]

Suggested change

	if len(badelements) > 0:
	if badelements:

[tay0thman]

done and applied the same to badcads and badrvts

[sanzoghenzo]

since you don't need the rest of the elements, just keep the first limit elements, no need to keep checking if you reach the limit

Suggested change

	output.print_md('{} Elements below are located more than {}'.format(BAD_STRG, CRITERIA_STRG))
	for x in badelements:
	print(output.linkify(x.Id)+ ' ' +
	str(x.Name) + ' ' + str(x.Category.Name))
	if counter == limit:
	break
	counter += 1
	badelements = badelements[:limit]
	output.print_md('{} Elements below are located more than {}'.format(BAD_STRG, CRITERIA_STRG))
	for x in badelements:
	print(output.linkify(x.Id)+ ' ' +
	str(x.Name) + ' ' + str(x.Category.Name))

[tay0thman]

such a beautiful trick