paramak icon indicating copy to clipboard operation
paramak copied to clipboard

Units usage not consistent

Open shimwell opened this issue 5 years ago • 12 comments

I thought the Paramak was kind of unit less as it just makes geometry with the numbers provided

However when writing to file and using the geometry upsstream in simulations units are required

CADQuery writes STEP files with units of milli meters

Neutronics operates in cm normally

I think the parametric plasma source uses meters

So I'm wondering if we should add units to the docstrings and if those units should be in meters? The actualy numbers might have to be internally scaled to milli meters but any thoughts on this are welcome

shimwell avatar Oct 15 '20 16:10 shimwell

Discussed with Remi

Perhaps addint and attribute for the units eg. 'm', 'cm', 'mm' is the way to go.

It looks like the CAD geometry is always going to be written in mm in the stp file, so I think we should scale the points to be in mm from whatever input the user provides

shimwell avatar Oct 15 '20 17:10 shimwell

@Shimwell when you say "CAD geometry is always going to be written in mm in the stp file" : i thought the stp files were dimensionless ?

As said earlier, internal scaling of the points attribute could do the trick. BUT, if the distance type attribute (eg. radius, height, inner_radius, minor_radius, major_radius, x_point, etc...) aren't scaled accordingly with the scale function, there might be some inconsistencies.

in order not to have this inconsistency between points and these attributes, one would need to add a tailored scaling method to every single parametric_component in the paramak: this is huge.

Moreover, as soon as the attributes of a class are modified, this scaling method should be modified accordingly --> many many tests are required to catch all eventual inconsistencies....

RemDelaporteMathurin avatar Oct 16 '20 07:10 RemDelaporteMathurin

This shows an example of a scaling method in paramak.Shape() which scales the points attributes coordinates. https://github.com/RemDelaporteMathurin/paramak/blob/84d7926259ed365e791ea792cd0c6dc807923e34/paramak/shape.py#L400-L411

https://github.com/RemDelaporteMathurin/paramak/blob/84d7926259ed365e791ea792cd0c6dc807923e34/paramak/utils.py#L7-L16

RemDelaporteMathurin avatar Oct 16 '20 07:10 RemDelaporteMathurin

CADQuery exporter and most exporters write the CAD file in mm and include things like this in the .STP file

SI_UNIT(.MILLI.,.METRE.)

I have tried to change these in the STP file (which can be just ASCII text) to SI_UNIT(.CENTI.,.METRE.) but it appears the either the PPP assumes mm or changing the units of a CAD file is more difficult than simply find and replace

Script for replacing units in a STP file, it didn't appear to work but this is what I tried

from tempfile import mkstemp
from shutil import move, copymode
from os import fdopen, remove

def _replace(self, filename, pattern, subst):
    """Opens a file and replaces occurances of a particular string
        (pattern)with a new string (subst) and overwrites the file.
        Used internally within the paramak to ensure .STP files are
        in units of cm not the default mm.
    Args:
        filename (str): the filename of the file to edit
        pattern (str): the string that should be removed
        subst (str): the string that should be used in the place of the 
            pattern string
    """
    #Create temp file
    fh, abs_path = mkstemp()
    with fdopen(fh,'w') as new_file:
        with open(filename) as old_file:
            for line in old_file:
                new_file.write(line.replace(pattern, subst))
    #Copy the file permissions from the old file to the new file
    copymode(filename, abs_path)
    #Remove original file
    remove(filename)
    #Move new file
    move(abs_path, filename)

replace('Graveyard_edit.stp', 'SI_UNIT(.MILLI.,.METRE.)', 'SI_UNIT(.CENTI.,.METRE.)')

shimwell avatar Oct 16 '20 08:10 shimwell

How about we stat units are in meters for every parametric component and parametric reactor in their doc strings. Then we don't need to add a units arg to those classes as we assume users have entered meters

shimwell avatar Oct 16 '20 08:10 shimwell

If they inherit from Shape() and Shape() has a units arg then they'll inherit the units arg I'm afraid.

RemDelaporteMathurin avatar Oct 16 '20 08:10 RemDelaporteMathurin

Ah yes ok so only reactors can be stated in meters.

shimwell avatar Oct 16 '20 10:10 shimwell

So:

  • reactors are stated in m
  • Parametric components have a units arguments (default in m)

points attribute can be scaled from the current unit to another unit. What should we do about the other distance arguments?

RemDelaporteMathurin avatar Oct 16 '20 10:10 RemDelaporteMathurin

I think the distance arguments used in operations like extrude should also be in m.

Quite a lot to keep track of

shimwell avatar Oct 16 '20 12:10 shimwell

then everything should be in m by default (reactor AND shapes). Therefore there is no need of a units attribute (since everything is in m)

The scaling function of the points could only be called when necessary (in the neutronics sim). If we want the users to be able to scale the points + the distance attributes at any time, a scaling method must be added and tailored to each and every class in the paramak 😄

RemDelaporteMathurin avatar Oct 16 '20 12:10 RemDelaporteMathurin

One option is to scale the solid, here is an example that uses CadQuery to load a stp file scale it and save the stp file.

import cadquery as cq
from cadquery import importers, exporters

a_new_part = importers.importStep('file.step')

a_new_part = a_new_part.val().scale(0.1)

with open('file_scaled.step', "w") as out_file:
    exporters.exportShape(a_new_part, "STEP", out_file)

shimwell avatar Dec 23 '20 22:12 shimwell

https://github.com/connorferster/forallpeople

Could this kind of package help ?

RemDelaporteMathurin avatar Feb 05 '21 07:02 RemDelaporteMathurin