Opened 14 years ago

Closed 13 years ago

#24 closed defect (fixed)

Clarify IM ParameterAPI/DependentParameterAPI ambiguity

Reported by: Kevin Milner Owned by: Peter Powers
Priority: major Milestone: OpenSHA 1.2
Component: sha Version:
Keywords: Cc: Ned Field, Peter Powers

Description

IntensityMeasureRelationship defines intensity measure as: protected ParameterAPI im; yet elsewhere we assume its a !DependentParameterAPI, such as in the method: IntensityMeasureRelationship.setIntensityMeasure(ParameterAPI intensityMeasure) (where it's cast to DependentParameterAPI). So far this hasn't been a problem because all thus-far implemented IMs are DependentParameterAPIs. Fix this either by changing the definition of im or putting and if statement before castings. We could either change the definition of im to be a DependentParameterAPI, or put an if statement before sections where we currently assume it's a DependentParameterAPI.

Change History (16)

comment:1 Changed 54 years ago by Kevin Milner

Milestone: OpenSHA 0.3

comment:2 Changed 54 years ago by Kevin Milner

Priority: majornormal

comment:3 Changed 54 years ago by Kevin Milner

Priority: normalmajor

comment:4 Changed 13 years ago by Kevin Milner

Cc: Ned Field Peter Powers added

Anyone object to me making this change (changing IM's in IntensityMeasureRelationshipAPI to DependentParameterAPI's)?

comment:5 Changed 13 years ago by Peter Powers

It's been a long time since I added this, but looking back I see a whole mess of casts. As long as nothing breaks, which it shouldn't I'm ok with you moving forward.

comment:6 Changed 13 years ago by Ned Field

OK by me

comment:7 Changed 13 years ago by Kevin Milner

OK I tried making the change, but it gets nasty REAL quick!

It seams like every (or at least almost every) parameter is already a dependent parameter. Can we just merge the two classes/interfaces, adding the functionality from DependentParameter? to Parameter?

comment:8 Changed 13 years ago by Peter Powers

To my mind, the existence of a DependentParameter? implies the existence of independent parameter, but there are none, and as you point out, everything is a DP. I'd guess that there was supposed to be some logic in the initial opensha code wherein there were some parameters with real dependencies and these class names would capture that. (e.g. any period parameter is dependent on having SA selected).

I'd say we should go ahead and combine them soon, but I think this is a major refactor that is going to touch many many classes and maybe we should put it off to 1.2. What do you think?

comment:9 Changed 13 years ago by Ned Field

A DependentParameter? simply means the parameter depends on other parameters, which are store in the IndependentParameterList? of the DependentParameter?.

I have no problem with making all parameters able to have such a list (it's just a pointer anyway). So yes, lets merge the two classes/interfaces into the parent.

That does raise the question of what to call the list (maybe the list should be called "DependentParameterList?", in that "the main parameter depends on the ones in this list". This might be less confusing that calling it the IndependentParameterList? since the parent is no longer called DependentParameter?.

Is this a "major refactor", and how are we gong to test things when it's done? I guess we just have to let people know and and ask them to keep and eye out for anything funning.

comment:10 Changed 13 years ago by Peter Powers

dependentParameters as the name of the internal field is better as it states what the contained objects are. Another appproach to consider might be calling the list 'children' to capture the dependency relationship. This is just a thought, but would be consistent with the parent-child relationships in xml, which we'll get to, eventually.

It's a major refactor not in terms of any functionality changes, but that the class name changes will occur in many many filesGiven the ubiquity of the Parameter classes, this can make it more difficult to merge changes from trunk onto a branch and vice-versa so I was advocating waiting until 1.2 so thhat we can give some advance warning to devs.

comment:11 Changed 13 years ago by Kevin Milner

Milestone: OpenSHA 1.1OpenSHA 1.2

I agree, let's hold off for 1.2.

comment:12 Changed 13 years ago by Kevin Milner

I preliminarily implemented this in a branch, branches/2011_05_parameter_refactor. Removed DependentParameter? in [7802] (and renamed some classes). I will close ticket when it's been tested more fully, cleaned up a bit, and merged back to trunk (and has the approval of Ned and Peter, of course). We may also want to rename some of the methods, as they were just moved from DependentParameter? to Parameter (as discussed above). Other cleanup may be necessary. Apps seem to work with the changes.

comment:13 Changed 13 years ago by Kevin Milner

OK I've finished the bulk of the changes and am ready to merge to trunk unless anyone objects/wants to wait. Do you want to change the method names related to independent parameters? Tests seem to be working so far (calculating curves, running the commons test suite).

comment:14 Changed 13 years ago by Ned Field

What name changes were you thinking?

comment:15 Changed 13 years ago by Peter Powers

Go for it.

comment:16 Changed 13 years ago by Kevin Milner

Resolution: fixed
Status: newclosed

Done, and merged to trunk with [7853]. We can still rename some of the methods, but we'll save that for another ticket.

Note: See TracTickets for help on using tickets.