Skip to content

Refactoring classes and properties - part 4 #832

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.

Sign up for GitHub

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
Merged
kmruehl merged 17 commits into from
Mar 28, 2022
Merged

Refactoring classes and properties - part 4 #832

kmruehl merged 17 commits into from
Mar 28, 2022

Conversation

@kmruehl
Copy link
Collaborator

@kmruehl kmruehl commented Mar 16, 2022 •
edited
Loading

This PR is a continuation of #803 and #822 and #828

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
    • Does @dforbush2 think this will be possible, given past discussions have been made on slowing down simulation time.
    • Yes, we have it working and have plans for some further cleanup that should help with speed
  • 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
  • waves.A to waves.amplitude
  • waves.S to waves.spectrum
  • waves.Pw to waves.power
  • waves.w to waves.omega
  • waves.dw to waves.dOmega
  • waves.k to waves.wavenumber

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
  • simu.g to simu.gravity

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 to body.b2bDOF
  • 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

Source/Functions/BEMIO

  • waveNumber to calcWaveNumber
  • spectralMoment to calcSpectralMoment

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
  • resolve wecSimPCT issue
  • move pctDir to simulationClass? No, decided to leave it as it since it's a modification of caseDir
  • how does b2b impact migration of passive yaw to bodyClass? Doesn't appear to be an issue, all passiveYaw tests are passing

@nathanmtom
Copy link

nathanmtom commented Mar 17, 2022 •
edited by kmruehl
Loading

Including @akeeste last comments on the previous PR for tracking, will add to and further comment here.

@kmruehl I've just finished reviewing the latest pull request, thanks for all of the effort here! I've left additional comments below. If you want assistance in implementing any of the changes or have questions on below happy to chat.

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 power, @akeeste and @nathanmtom this differs from prior suggestions. We can include "per width" in the documentation, doesn't need to be in the variable name itself.
  • w to omega, @akeeste and @nathanmtom this differs from prior suggestions. omega is a calculated parameter, is industry standard terminology, and it's now consistent with our documentation, e.g. omega = 2pi/T or 2pi*frequency
  • dw to dOmega, @akeeste and @nathanmtom this differs from prior suggestions.
  • k to wavenumber From @nathanmtom Also suggest adding to description after % to state the wave number is 2*\pi / wavelength and is in the units of [rad/m] or since rad is unitless just [1/m].

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 @nathanmtom for now we're going to leave this as is, because pctDir is an automated revision to caseDir

Body Class
From @nathanmtom

  • volume to displacedVolume Although volume is simpler this could be the entire volume of the floating body and not just the displaced volume that contributes to buoyancy and hydrostatic effects. For a hydrodynamic body this is pulled from the .h5 file, but even a drag or non-hydro body is expecting the displaced volume. @nathanmtom this can be addressed in the documentation, not in the variable naming
  • dofCoupled to b2bDOF to follow the convention for gbmDOF
  • Swap lines for centerGravity and centerBuoyancy to follow alphabetical listing.

Simulation Class
From @nathanmtom

  • g to gravity

Cable Class
From @nathanmtom

  • volume to displacedVolume Same reasoning behind case for the bodyClass @nathanmtom this can be addressed in the documentation, not in the variable naming
  • base.name based on the variable name, I would have expected it to be initialized as an empty string, but we have a [999, 999, 999]). Also, we will need to update the comments to reflect the variables are in a structure. @nathanmtom this must have been in a prior revision, because cable.base.name = 'NOT DEFINED' by default

PTO/Constraint Class
From @nathanmtom

  • Unrelated to refactoring, but when we have an initial displacement for the pto and constraint classes this is just shifting the location not actually giving the constraint or pto an initial rotation from say free decay. There were some issues in the past and this was circumvented by using a set placement within the pto block. This maybe a conversation worth having in the future to correct this misconception. @nathanmtom I agree, we should add this to the project board

Class setAccess and getAccess resolved with #838:
This PR addresses some refactoring items, namely updates the setAccess and getAccess of various class properties based on how they are used. Some setX() methods are added so that properties can be private-public while still set in initializeWecSim

  • make number properties private-public.
  • Add setNumber() methods so that number properties can still be set in initializeWecSim
  • Remove mooring.orientation as it is not used anywhere. Note, this is not used in the same way as the other classes
  • 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 update the docstring so that the API pulls the full description
  • body.dofCoupled, body.total to private-public and defined in a body.setup() method
  • remove body.hydroTotal, not used
  • body.massCalcMethod to private-public and removed from initializeWecSim line 149. This should not be called there
  • simu.cicTime, simu.cicLength, simu.caseFile, simu.date, simu.gitCommit, simu.maxIt, simu.outputDir, simu.wsVersion to private-public, not used outside of the simulationClass
  • remove simu.inputFile, simu.logFile, not used

Help / documentation:
NOTE: This will all be done by the team in another PR. Tasks will be assigned to different team members by Class. Refer to https://github.com/WEC-Sim/WEC-Sim/projects/58#card-79798675

  • add dev documentation for style guide, look into naming convention, e.g. pep 8
  • 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 all classes now that properties and methods have changed.
    • waveClass
    • simulationClass
    • bodyClass
    • ptoClass
    • constraintClass
    • mooringClass
    • cableClass

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.

@kmruehl kmruehl mentioned this pull request Mar 23, 2022
Merged
10 tasks
@kmruehl kmruehl added the SCM source code mangagement and warnings label Mar 23, 2022
@kmruehl kmruehl mentioned this pull request Mar 23, 2022
Merged
@kmruehl
Copy link
Collaborator Author

kmruehl commented Mar 25, 2022

@akeeste can you confirm that your prior comments noted in the above comment under Class setAccess and getAccess: resolved with #838 have been resolved?

@akeeste
Copy link
Contributor

akeeste commented Mar 25, 2022

@akeeste can you confirm that your prior comments noted in the above comment under Class setAccess and getAccess: resolved with #838 have been resolved?

@kmruehl Yes all the getAccess and setAccess comments are resolved

@kmruehl
Copy link
Collaborator Author

kmruehl commented Mar 28, 2022 •
edited
Loading

A few more source/functions/bemio/ changes:

  • waveNumber to calcWaveNumber
  • spectralMoment to calcSpectralMoment

@kmruehl
Copy link
Collaborator Author

kmruehl commented Mar 28, 2022

@H0R5E why are doc builds failing?

@kmruehl kmruehl added Mooring Class Mooring Class (mooringClass.m) PTO Class Power Take-off Class (ptoClass.m) Simulation Class Simulation Class (simulationClass.m) Response Class Response Class (responseClass.m) Body Class Body Class (bodyClass.m) Constraint Class Constraint Class (constraintClass.m) Library updates to the WEC-Sim Library Cable Class Cable Class (cableClass.m) labels Mar 28, 2022
@kmruehl kmruehl added the Wave Class Wave Classs (waveClass.m) label Mar 28, 2022
@kmruehl
Copy link
Collaborator Author

kmruehl commented Mar 28, 2022

@nathanmtom and @akeeste all of your feedback has been addressed and included in this PR. There are a few instances where I made some revisions to the suggested property names, and/or suggested we update the documentation instead of the property name. I'm not sure why the docs are building, but I plan to merge this with dev shortly and open a new PR to address the outstanding documentation tasks.

@kmruehl kmruehl merged commit fd6ca7c into WEC-Sim:dev Mar 28, 2022
@H0R5E
Copy link
Contributor

H0R5E commented Mar 29, 2022

@H0R5E why are doc builds failing?

See #839

@kmruehl kmruehl mentioned this pull request Mar 29, 2022
Merged
@kmruehl kmruehl deleted the refactoring4 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

No reviews

Assignees

No one assigned

Labels

Body Class Body Class (bodyClass.m) Cable Class Cable Class (cableClass.m) Constraint Class Constraint Class (constraintClass.m) Library updates to the WEC-Sim Library 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) Wave Class Wave Classs (waveClass.m)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kmruehl @nathanmtom @akeeste @H0R5E