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
Feature: check_total_derivatives #306
Conversation
openmdao/core/problem.py
Outdated
@@ -33,6 +35,10 @@ | |||
ErrorTuple = namedtuple('ErrorTuple', ['forward', 'reverse', 'forward_reverse']) | |||
MagnitudeTuple = namedtuple('MagnitudeTuple', ['forward', 'reverse', 'fd']) | |||
|
|||
# when setup is called multiple times, we need this to prevent adding | |||
# another handler to the config_check logger each time (if logger arg to check_config is None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment needs updated for this module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
openmdao/core/problem.py
Outdated
logger.addHandler(console) | ||
else: | ||
logger = _set_logger | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is duplicated.. might make it a module level function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, not quite duplicated, except I forgot to change the logger name when I copied it. I don't mind the duplication, but if we remove it, we also need to remove the code in check_config. That code is slightly different in that it specifies a custom format for the messages that are logged. Perhaps we need a new story for a unified logging manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the only diff is the logger name, that's just an arg to the function.. it is quite a bit of duplication IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
logger.info(str(fd)) | ||
logger.info('') | ||
|
||
logger.info(' -' * 30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make sense to move some of this code to another file.. since half of the problem.py
module now seems to consist of "check derivatives" logic, maybe it warrants it's own module similar to check_config
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I am not that worried about it, and half of the code (check_partials) is probably moving into Group or System sometime anyway. If there is enough of a demand for splitting out the check codes into a separate module, then it can be done as a separate story.
compact_print, comps, global_options) | ||
|
||
return partials_data | ||
|
||
def check_total_derivatives(self, of=None, wrt=None, logger=None, compact_print=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should edit the README.md file and remove 'Total Derivatives checking' from the list of things we don't support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
check_total_derivatives
added to Problem. This method checks the derivatives of the driver constraints and objectives with respect to desvars, and compares them to fd or cs as chosen. It can also be used to compare user selected 'of' and 'wrt.check_partials
converted over to use logger.INFO.