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

Jump to bottom

Refactoring classes and properties - part 3 #828

Merged
merged 17 commits into from Mar 16, 2022
Merged

Refactoring classes and properties - part 3 #828

merged 17 commits into from Mar 16, 2022

Conversation

kmruehl
Copy link
Contributor

@kmruehl kmruehl commented Mar 15, 2022 •
edited

This PR is a continuation of #803 and #822

Objectives:

  • clear and intuitive variable naming using camelCase
  • consistent properties across classes, e.g. pto.loc versus mooring.ref
  • convert properties for a single feature to property structures, e.g. struct(body.morison)
  • move properties to the appropriate class, e.g. simu.yaw to body.yaw
  • updating WEC-Sim library accordingly

Wave Class

  • waves.etaImport to waves.waveImport to waves.elevationImport
  • waves.spectrumDataFile to waves.waveSpectrumFile to waves.spectrumFile
  • waves.etaDataFile to waves.waveElevationFileto waves.elevationFile
  • waves.plotEta to waves.plotElevation
  • waves.waveDir to waves.waveDirection to waves.direction
  • waves.waveSpread to waves.spread
  • beta to theta to match online documentation
  • waves.deepWaterWave to waves.deepWater
  • convert waves.current properties to structure, similar to morison element,
    • waves.currentOption to waves.current.option
    • waves.currentDepth to waves.current.depth
    • waves.currentDirection to waves.current.direction
    • waves.currentSpeed to waves.current.speed
  • convert properties related to BEM data to waves.bem structure
    • waves.numFreq to waves.freqNum to waves.bem.count
    • waves.freqDisc to waves.bem.option
    • waves.freqRange to wave.bem.range
  • add checks for waveClass structures
  • convert waves.marker properties to structure
    • waves.markerLoc to waves.marker.loc to waves.marker.location
    • waves.markerSize to waves.marker.size
    • waves.markerStyle to waves.marker.style
    • convert waves.marker properties to structure in Frames Library
  • waves.T to waves.period
  • waves.H to waves.height

Simulation Class

  • simu.writetxt to simu.writeText to simu.saveText
  • waves.waveStatisticsDataLoad to simu.mcrExcelFile (formerly Wave Class)
  • simu.mcrCaseFile to simu.mcrMatFile
  • simu.dtME to simu.morisonDt
  • simu.CTTime to simu.cicTime
  • simu.dtCI to simu.cicDt
  • simu.CITime to simu.cicEndTime
  • simu.CIkt to simu.cicLength
  • simu.ssCalc to simu.stateSpace
  • simu.setupSimu to simu.simuSetup to simu.setup
  • convert simu.paraview properties to structure, similar to morison element, note: test that sub-properties are tested
    • simu.dtParaview to simu.paraviewDt to simu.paraview.dt
    • simu.StartTimeParaview to simu.paraviewStartTime to simu.paraview.startTime
    • simu.EndTimeParaview to simu.paraviewEndTime to simu.paraview.endTime
    • simu.pathParaviewVideo to simu.paraviewDirectory to simu.paraview.path
  • simu.numWecBodies to simu.body.numHydroBodies
  • simu.pressureDis to simu.pressure
  • simu.simulationDate to simu.date
  • add checks for simulationClass structures
  • simu.adjMassWeightFun to simu.adjMassFactor
  • simu.autoRateTranBlk to simu.rateTransition
  • simu.zeroCrossCont to simu.zeroCross
  • simu.numIntMidTimeSteps removed
  • parallelComputing_dir to pctDir NOTE this is not in the simulationClass, but maybe it should be

Body Class

  • body.dof_start to body.dofStart
  • body.dof_end to body.dofEnd
  • body.dof_gbm to body.dofGBM to body.gbmDOF
  • body.nlHydro to body.nonlinearHydro
  • body.lenJ to body.dofCoupled
  • body.meanDriftForce to body.meanDrift
  • body.bodyparaview to body.paraviewBody to body.paraview
  • body.flexHydroBody to body.flexBody to body.flex
  • body.nhBody to body.nonHydroBody to body.nonHydro
  • body.hydroDataBodyNum to body.hydroBodyTotal to body.hydroTotal NOTE Do we even use this?
  • body.viscDrag to body.viscousDrag to body.quadDrag
  • body.userDefinedExcIRF to body.excitationIRF
  • add checks for bodyClass structures
  • move simu.yaw and simu.yawThresh to bodyClass
    • simu.yaw to body.yaw.option
    • simu.yawThres to body.yaw.threshold
  • body.bodyNumber to body.number
  • body.bodyTotal to body.total
  • body.bodyGeometry to body.geometry
    from @nathanmtom
  • quadDrag.Drag to quadDrag.drag
  • morisonElement.characteristicArea to morisonElement.area
  • quadDrag.characteristicArea to quadDrag.area
  • dispVol to displacedVolume to volume
  • body.initDisp.initLinDisp to body.initDisp.initLinearDisp to body.initial.displacement
  • body.initDisp.initAngularDispAngle to body.initial.angle
  • body.initDisp.initAngularDispAxis to body.initial.axis
  • cg to centerGravity
  • cb to centerBuoyancy
  • momOfInertia to intertia

Cable Class

  • cable.c to cable.damping
  • cable.k to cable.stiffness
  • cable.viscDrag to cable.viscousDrag to cable.quadDrag
  • cable.cableNum to cable.number
  • cable.loc to cable.location
    from @nathanmtom
  • cable.dispVol to cable.displacedVolume to cable.volume
  • cable.quadDrag.characteristicArea to quadDrag.area
  • cable.quadDrag.Drag to quadDrag.drag
  • cable.initDisp.initLinDisp to cable.initDisp.initLinearDisp to cable.initial.displacement
  • cable.initDisp.initAngularDispAngle to cable.initial.angle
  • cable.initDisp.initAngularDispAxis to cable.initial.axis
  • cable.L0 to cable.unstretchedLength tp cable.length
  • cable.bodyMass to cable.mass
  • cable.bodyInertia to cable.momOfInertia to cable.intertia
  • convert to cable.base and cable.follower structure
    • cable.cb1 to cable.centerBuoyancyBase to cable.baseCb to cable.base.centerBuoyancy
    • cable.cg1 to cable.centerGravityBase to cable.baseCg to cable.base.centerGravity
    • cable.rotloc1 to cable.rotationLocation1 to cable.baseLocation to to cable.base.location
    • cable.baseConnectionName to cable.base.name
    • cable.cb2 to cable.centerBuoyancyFollower to cable.followerCb to cable.follower.centerBuoyancy
    • cable.cg2 to cable.centerGravityFollower to cable.followerCg to cable.follower.centerGravity
    • cable.rotloc2 to cable.rotationLocation2 to cable.followerLocation to cable.follower.location
    • cable.followerConnectionName to cable.follower.name

Constraint Class

  • constraint.loc to constraint.location
  • constraint.constraintNum to constraint.number
    from @nathanmtom
  • constraint.initDisp.initLinDisp to constraint.initDisp.initLinearDisp to constraint..initial.displacement

PTO Class

  • pto.loc to pto.location
  • pto.c to pto.damping
  • pto.k to pto.stiffness
  • pto.ptoNum to pto.number
    from @nathanmtom
  • pto.initDisp.initLinDisp to pto.initDisp.initLinearDisp to pto.initial.displacement

Mooring Class

  • mooring.ref to mooring.loc to mooring.location
  • mooring.loc to mooring.orientation (internal)
  • mooring.mooringNum to mooring.number
    from @nathanmtom
  • mooring.initDisp.initLinDisp to mooring.initDisp.initLinearDisp to mooring.initial.displacement
  • mooring.initDisp.initAngularDispAngle to mooring.initial.angle
  • mooring.initDisp.initAngularDispAxis to mooring.initial.axis

Doc

  • add dev documentation for style guide, look into naming convention, e.g. pep 8

Notes

  • Are the property structure fields checked? No, properties are automatically checked, but not the structure fields. For example there's an error if waves.bems is specified in the input file, but not for waves.bem.options. This can be addressed by adding some additional checks to the checkinputs method of each class with a structure.
  • resolve TestPassiveYawRegression test failure
  • how does b2b impact migration of passive yaw to bodyClass?
  • resolve wecSimPCT issue
  • move pctDir to simulationClass?

@kmruehl kmruehl mentioned this pull request Mar 15, 2022
Merged
26 tasks
@kmruehl
Copy link
Contributor Author

kmruehl commented Mar 15, 2022 •
edited

Review from @nathanmtom refer to #822 (comment) for full (unedited) review

Working review comments:

Body Class

  • In the original comment of this PR there is body.dof_gbm to body.dofGBM but in the simulationClass it appears to be gbmDOF no issue with this labeling, but just confirming intent.) kmruehl: my mistake, it should be gbmDOF, I updated this

Wave Class

  • waves.T to waves.period (another case where if we are consistent with using full names than assumed variables we should change; however, I'd say this is pretty standard in our industry and do not think we have had any confusion here.) consensus with team
  • waves.H to waves.height (another case where if we are consistent with using full names than assumed variables we should change; however, I'd say this is pretty standard in our industry and do not think we have had any confusion here.)
    consensus with team

Simulation Class

  • simu.writeText seems to have been removed, just confirming intention here. kmruehl: good catch, this was updated to simu.saveText, my apologies. I've updated this above

Cable Class

  • Clarify that linearDamping and quadDamping in the cable class is related to the global motion of the cable, not associated with extension of the cable? dforbush: linear damping applies to global cable motion at the drag bodies

Other Comments

  • Side comment here, but outside of it being difficult to maintain, it does seem like having a list of all variables in a table and every location they are used would be valuable to make similar changes in the future. Also, this allows to double check if a variable is actually used or not. I would appreciate some thoughts on if the team can think of a good method for this.
  • akeeste: what about other properties, e.g. waveClass private/public, e.g. waves.A

@kmruehl kmruehl added the SCM source code mangagement and warnings label Mar 16, 2022
@kmruehl kmruehl added Mooring Class Mooring Class (mooringClass.m) PTO Class Power Take-off Class (ptoClass.m) MoorDyn MoorDyn implementation in WEC-Sim Simulation Class Simulation Class (simulationClass.m) Response Class Response Class (responseClass.m) Body Class Body Class (bodyClass.m) Constraint Class Constraint Class (constraintClass.m) Cable Class Cable Class (cableClass.m) labels Mar 16, 2022
@kmruehl
Copy link
Contributor Author

kmruehl commented Mar 16, 2022

@H0R5E do you know why the tests stopped running on this PR?

@H0R5E
Copy link
Contributor

H0R5E commented Mar 16, 2022

@kmruehl, no, I don't know why. V strange.

@kmruehl
Copy link
Contributor Author

kmruehl commented Mar 16, 2022

@kmruehl, no, I don't know why. V strange.

indeed...

@H0R5E
Copy link
Contributor

H0R5E commented Mar 16, 2022

Actions are degraded. You'll need to wait for a fix.

@akeeste
Copy link
Contributor

akeeste commented Mar 16, 2022 •
edited by kmruehl

A few more comments on our refactoring:

Wave Class
I notice that the only places we use short industry-standard letters instead of words for variable naming is the internal private-public part of the wave class (aside from BEMIO and the hydro struct). These are not input file variables but it seems feasible to update these last few before v5.0 for consistency. My proposal:

  • A to amplitude
  • S to spectrum
  • Pw to powerPerWidth
  • dw to dFreq or df or could be left as dw (like dt)
  • k to wavenumber
  • w to frequency

Simulation Class
pctDir is appended onto simu.caseDir when wecSimPCT is used, so we probably don't have to save it as a separate variable

Class setAccess and getAccess:

  • the number property is almost always public-public because it's set in initializeWecSim but it should not be set by users. We could create simple setNumber() methods so that they can be private-public
  • mooring.orientation is not used anywhere. It's not used in the same way as the other classes
  • pto, constraint, mooring are otherwise straightforward in this regard
  • response no changes needed here
  • move waves.type to private-public. typeNum is set privately and only in the class constructor so type must be set through the constructor as well
  • body.geometry, body.h5File to private-public and probably need to redo the docstring so that the API pulls the full description (like body.morisonElement
  • body.dofCoupled, body.total to private-public and set in a method like checkInputs
  • body.hydroTotal not used
  • body.massCalcMethod to private-public and should likely be removed from initializeWecSim line 149, I dont' see why this parameter should be set there
  • simu.cicTime, simu.cicLength, simu.caseFile, simu.date, simu.gitCommit, simu.maxIt, simu.outputDir, simu.wsVersion aren't used outside of the simulationClass, move to private-public
  • simu.inputFile, simu.logFile aren't used
  • simu.numX used in initializeWecSim but could be moved to a setNum method and be private-public

Help / documentation:
We should revisit how to format docstrings for class structures. Right now the API is messy to read and the help and doc functions don't list all the necessary information.

  • When using doc XClass or doc XClass.method the constructor only lists the first line of the docstring. "Parameters" and "Returns" are not shown in the matlab docs. The API (usually) builds this description correctly. We might consider how to make doc and the API work well together, or pick one to point users to.
  • update .cg and .cb in .rst files

@kmruehl
Copy link
Contributor Author

kmruehl commented Mar 16, 2022

@akeeste and @nathanmtom This branch is currently stable and passing all tests so I'm going to merge this PR and open a new one to address the remaining comments.

@kmruehl kmruehl merged commit 8e88edb into WEC-Sim:dev Mar 16, 2022
@kmruehl kmruehl mentioned this pull request Mar 16, 2022
Merged
H0R5E pushed a commit to H0R5E/WEC-Sim that referenced this pull request Mar 17, 2022
@kmruehl @H0R5E
Refactoring classes and properties - part 3 (WEC-Sim#828)
cde78fc 
* body and cable dispVol to volume

* initDisp to initial

* initLinDisp to displacement initAngularDispAngle to angle initAngularDispAxis to axis

* save library to 2020b

* cable rotLoc1 and rotLoc2 to baseLocation and followerLocation

* cable cg1 cb1 to baseCg baseCg, cg2 cb2 to followerCg and followerCb

* cable bodyMass and bodyInertia to mass and momOfInertia

* class formatting

* waves.T to waves.period and waves.H to waves.height

* resolve waveClass runfromSimulink bug

* body.cg to body.centerGravity

* library to 2020b

* body.cb to body.centerBuoyancy

* library to 2020b

* bodyClass and cableClass momOfInertia to intertia

* convert cableClass to base and follower structures
@kmruehl kmruehl mentioned this pull request Mar 23, 2022
Merged
26 tasks
@kmruehl
Copy link
Contributor Author

kmruehl commented Mar 23, 2022

All outstanding comments from this PR have mean migrated to #832 for resolution

@kmruehl kmruehl mentioned this pull request Mar 29, 2022
Merged
@kmruehl kmruehl deleted the refactoring branch May 17, 2023 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akeeste akeeste Awaiting requested review from akeeste

@nathanmtom nathanmtom Awaiting requested review from nathanmtom

Assignees
No one assigned
Labels
Body Class Body Class (bodyClass.m) Cable Class Cable Class (cableClass.m) Constraint Class Constraint Class (constraintClass.m) MoorDyn MoorDyn implementation in WEC-Sim Mooring Class Mooring Class (mooringClass.m) PTO Class Power Take-off Class (ptoClass.m) Response Class Response Class (responseClass.m) SCM source code mangagement and warnings Simulation Class Simulation Class (simulationClass.m)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kmruehl @H0R5E @akeeste